From ada82b840f642681f8012cb2a46aa7577393dd30 Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Sat, 20 Jun 2020 10:51:21 +0200 Subject: [PATCH 1/8] Get update from tinyusb. --- src/class/hid/hid_device.c | 4 +++- src/class/msc/msc_device.c | 6 ++++-- src/device/usbd.c | 21 +++++++++---------- src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c | 1 - 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/class/hid/hid_device.c b/src/class/hid/hid_device.c index 2a20bc17..526394d4 100644 --- a/src/class/hid/hid_device.c +++ b/src/class/hid/hid_device.c @@ -194,7 +194,9 @@ uint16_t hidd_open(uint8_t rhport, tusb_desc_interface_t const * desc_itf, uint1 p_hid->boot_mode = false; // default mode is REPORT p_hid->itf_num = desc_itf->bInterfaceNumber; - memcpy(&p_hid->report_desc_len, &(p_hid->hid_descriptor->wReportLength), 2); + + // Use offsetof to avoid pointer to the odd/misaligned address + memcpy(&p_hid->report_desc_len, (uint8_t*) p_hid->hid_descriptor + offsetof(tusb_hid_descriptor_hid_t, wReportLength), 2); // Prepare for output endpoint if (p_hid->ep_out) diff --git a/src/class/msc/msc_device.c b/src/class/msc/msc_device.c index b038d00f..3ddcd4e4 100644 --- a/src/class/msc/msc_device.c +++ b/src/class/msc/msc_device.c @@ -80,7 +80,8 @@ static inline uint32_t rdwr10_get_lba(uint8_t const command[]) // copy first to prevent mis-aligned access uint32_t lba; - memcpy(&lba, &p_rdwr10->lba, 4); + // use offsetof to avoid pointer to the odd/misaligned address + memcpy(&lba, (uint8_t*) p_rdwr10 + offsetof(scsi_write10_t, lba), 4); // lba is in Big Endian format return tu_ntohl(lba); @@ -93,7 +94,8 @@ static inline uint16_t rdwr10_get_blockcount(uint8_t const command[]) // copy first to prevent mis-aligned access uint16_t block_count; - memcpy(&block_count, &p_rdwr10->block_count, 2); + // use offsetof to avoid pointer to the odd/misaligned address + memcpy(&block_count, (uint8_t*) p_rdwr10 + offsetof(scsi_write10_t, block_count), 2); return tu_ntohs(block_count); } diff --git a/src/device/usbd.c b/src/device/usbd.c index 3708a9af..03ef655b 100644 --- a/src/device/usbd.c +++ b/src/device/usbd.c @@ -437,7 +437,7 @@ void tud_task (void) if ( 0 == epnum ) { - usbd_control_xfer_cb(event.rhport, ep_addr, event.xfer_complete.result, event.xfer_complete.len); + usbd_control_xfer_cb(event.rhport, ep_addr, (xfer_result_t)event.xfer_complete.result, event.xfer_complete.len); } else { @@ -445,7 +445,7 @@ void tud_task (void) TU_ASSERT(drv_id < USBD_CLASS_DRIVER_COUNT,); TU_LOG2(" %s xfer callback\r\n", _usbd_driver[drv_id].name); - _usbd_driver[drv_id].xfer_cb(event.rhport, ep_addr, event.xfer_complete.result, event.xfer_complete.len); + _usbd_driver[drv_id].xfer_cb(event.rhport, ep_addr, (xfer_result_t)event.xfer_complete.result, event.xfer_complete.len); } } break; @@ -763,11 +763,9 @@ static bool process_set_config(uint8_t rhport, uint8_t cfg_num) // If IAD exist, assign all interfaces to the same driver if (desc_itf_assoc) { - // IAD's first interface number and class/subclass/protocol should match with opened interface - TU_ASSERT(desc_itf_assoc->bFirstInterface == desc_itf->bInterfaceNumber && - desc_itf_assoc->bFunctionClass == desc_itf->bInterfaceClass && - desc_itf_assoc->bFunctionSubClass == desc_itf->bInterfaceSubClass && - desc_itf_assoc->bFunctionProtocol == desc_itf->bInterfaceProtocol); + // IAD's first interface number and class should match with opened interface + TU_ASSERT(desc_itf_assoc->bFirstInterface == desc_itf->bInterfaceNumber && + desc_itf_assoc->bFunctionClass == desc_itf->bInterfaceClass); for(uint8_t i=1; ibInterfaceCount; i++) { @@ -848,8 +846,10 @@ static bool process_get_descriptor(uint8_t rhport, tusb_control_request_t const if (!tud_descriptor_bos_cb) return false; tusb_desc_bos_t const* desc_bos = (tusb_desc_bos_t const*) tud_descriptor_bos_cb(); + uint16_t total_len; - memcpy(&total_len, &desc_bos->wTotalLength, 2); // possibly mis-aligned memory + // Use offsetof to avoid pointer to the odd/misaligned address + memcpy(&total_len, (uint8_t*) desc_bos + offsetof(tusb_desc_bos_t, wTotalLength), 2); return tud_control_xfer(rhport, p_request, (void*) desc_bos, total_len); } @@ -863,7 +863,8 @@ static bool process_get_descriptor(uint8_t rhport, tusb_control_request_t const TU_ASSERT(desc_config); uint16_t total_len; - memcpy(&total_len, &desc_config->wTotalLength, 2); // possibly mis-aligned memory + // Use offsetof to avoid pointer to the odd/misaligned address + memcpy(&total_len, (uint8_t*) desc_config + offsetof(tusb_desc_configuration_t, wTotalLength), 2); return tud_control_xfer(rhport, p_request, (void*) desc_config, total_len); } @@ -917,8 +918,6 @@ static bool process_get_descriptor(uint8_t rhport, tusb_control_request_t const default: return false; } - - return true; } //--------------------------------------------------------------------+ diff --git a/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c b/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c index c5666421..b539f479 100644 --- a/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c +++ b/src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c @@ -698,7 +698,6 @@ bool dcd_edpt_open (uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc default: TU_ASSERT(false); - return false; } pcd_set_eptype(USB, epnum, wType); From 349c0f640e7d4532cc96fd28dea2dd11a5d43bde Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Mon, 14 Sep 2020 18:24:08 +0200 Subject: [PATCH 2/8] Implementation done, yet to be tested. --- src/common/tusb_fifo.c | 421 ++++++++++++++++++++++++++++++++--------- src/common/tusb_fifo.h | 71 ++++--- 2 files changed, 362 insertions(+), 130 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index 6ab158cc..4e274003 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -2,6 +2,7 @@ * The MIT License (MIT) * * Copyright (c) 2019 Ha Thach (tinyusb.org) + * Copyright (c) 2020 Reinhard Panhuber * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -64,51 +65,288 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si f->item_size = item_size; f->overwritable = overwritable; - f->rd_idx = f->wr_idx = f->count = 0; + f->non_used_index_space = ((2^16)-1) % depth; + f->max_pointer_idx = ((2^16)-1) - f->non_used_index_space; + + f->rd_idx = f->wr_idx = 0; tu_fifo_unlock(f); return true; } +// TODO: To be changed!! static inline uint16_t _ff_mod(uint16_t idx, uint16_t depth) { return (idx < depth) ? idx : (idx-depth); } -// retrieve data from fifo -static inline void _ff_pull(tu_fifo_t* f, void * buffer, uint16_t n) +// send one item to FIFO WITHOUT updating write pointer +static inline void _ff_push(tu_fifo_t* f, void const * data, uint16_t n, uint16_t wRel) { - memcpy(buffer, - f->buffer + (f->rd_idx * f->item_size), - f->item_size*n); - - f->rd_idx = _ff_mod(f->rd_idx + n, f->depth); - f->count -= n; + memcpy(f->buffer + (wRel * f->item_size), data, n*f->item_size); } -// send data to fifo -static inline void _ff_push(tu_fifo_t* f, void const * data, uint16_t n) +// send n items to FIFO WITHOUT updating write pointer +static void _ff_push_n(tu_fifo_t* f, void const * data, uint16_t n, uint16_t wRel) { - memcpy(f->buffer + (f->wr_idx * f->item_size), - data, - f->item_size*n); - - f->wr_idx = _ff_mod(f->wr_idx + n, f->depth); - - if (tu_fifo_full(f)) + if(wRel + n <= f->depth) // Linear mode only { - f->rd_idx = f->wr_idx; // keep the full state (rd == wr && count = depth) + memcpy(f->buffer + (wRel * f->item_size), data, n*f->item_size); + } + else // Wrap around + { + uint16_t nLin = f->depth - wRel; + + // Write data to linear part of buffer + memcpy(f->buffer + (wRel * f->item_size), data, nLin*f->item_size); + + // Write data wrapped around + memcpy(f->buffer, data + nLin*f->item_size, (n - nLin) * f->item_size); + } +} + +// get one item from FIFO WITHOUT updating write pointer +static inline void _ff_pull(tu_fifo_t* f, void const * p_buffer, uint16_t rRel) +{ + memcpy(p_buffer, f->buffer + (rRel * f->item_size), f->item_size); +} + +// get n items from FIFO WITHOUT updating write pointer +static void _ff_pull_n(tu_fifo_t* f, void const * p_buffer, uint16_t n, uint16_t rRel) +{ + if(rRel + n <= f->depth) // Linear mode only + { + memcpy(p_buffer, f->buffer + (rRel * f->item_size), n*f->item_size); + } + else // Wrap around + { + uint16_t nLin = f->depth - rRel; + + // Read data from linear part of buffer + memcpy(p_buffer, f->buffer + (rRel * f->item_size), nLin*f->item_size); + + // Read data wrapped part + memcpy(p_buffer + nLin*f->item_size, f->buffer, (n - nLin) * f->item_size); + } +} + +// Advance an absolute pointer +static uint16_t advance_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) +{ + // We limit the index space of p such that a correct wrap around happens + // Check for a wrap around or if we are in unused index space - This has to be checked first!! We are exploiting the wrap around to the correct index + if ((p > p + pos) || (p + pos >= f->max_pointer_idx)) + { + p = (p + pos) + f->non_used_index_space; } else { - f->count += n; + p += pos; } + return p; +} + +// get relative from absolute pointer +static uint16_t get_relative_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) +{ + return _ff_mod(advance_pointer(f, p, pos), f->depth); +} + +// Works on local copies of w and r +static uint16_t _tu_fifo_count(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) +{ + uint16_t cnt = wAbs-rAbs; + + // In case we have non-power of two depth we need a further modification + if (rAbs > wAbs) cnt -= f->non_used_index_space; + + return cnt; +} + +// Works on local copies of w and r +static inline bool _tu_fifo_empty(uint16_t wAbs, uint16_t rAbs) +{ + return wAbs == rAbs; +} + +// Works on local copies of w and r +static inline bool _tu_fifo_full(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) +{ + return (_tu_fifo_count(f, wAbs, rAbs) == f->depth); +} + +// Works on local copies of w and r +// BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" +// EXAMPLE with buffer depth: 100 +// Maximum index space: (2^16)-1) - ((2^16)-1) % depth = 65500 +// If you produce 65500 / 100 = 655 buffer overflows, the write pointer will overflow as well and +// the check _tu_fifo_overflow() will not give you a valid result! Avoid such nasty things! +// Use _tu_fifo_correct_read_pointer() if overflow happened to set read pointer to correct index +// for reading latest items! +static inline bool _tu_fifo_overflow(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) +{ + return (_tu_fifo_count(f, wAbs, rAbs) > f->depth); +} + +// Works on local copies of w +// For more details see _tu_fifo_overflow()! +static inline void _tu_fifo_correct_read_pointer(tu_fifo_t* f, uint16_t wAbs) +{ + tu_fifo_lock(f); + f->rd_idx = advance_pointer(f, f->wr_idx, 1); + tu_fifo_unlock(f); +} + +// Works on local copies of w and r +static bool _tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t wAbs, uint16_t rAbs) +{ + uint16_t cnt = _tu_fifo_count(f, wAbs, rAbs); + + // Skip beginning of buffer + if (cnt == 0 || pos >= cnt) return false; + + uint16_t rRel = get_relative_pointer(f, rAbs, pos); + + // Peek data + _ff_pull(f, p_buffer, rRel); + + return true; +} + +// Works on local copies of w and r +static uint16_t _tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n, uint16_t wAbs, uint16_t rAbs) +{ + uint16_t cnt = _tu_fifo_count(f, wAbs, rAbs); + + // Skip beginning of buffer + if (cnt == 0 || pos >= cnt) return 0; + + // Check if we can read something at and after pos - if too less is available we read what remains + cnt -= pos; + if (cnt < n) { + if (cnt == 0) return 0; + n = cnt; + } + + uint16_t rRel = get_relative_pointer(f, rAbs, pos); + + // Peek data + _ff_pull_n(f, p_buffer, n, rRel); + + return n; +} + +// Works on local copies of w and r +static inline uint16_t _tu_fifo_remaining(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) +{ + return f->depth - _tu_fifo_count(f, wAbs, rAbs); } /******************************************************************************/ /*! - @brief Read one element out of the RX buffer. + @brief Get number of items in FIFO. + + As this function only reads the read and write pointers once, this function is + reentrant and thus thread and ISR save without any mutexes. + + @param[in] f + Pointer to the FIFO buffer to manipulate + + @returns Number of items in FIFO + */ +/******************************************************************************/ +uint16_t tu_fifo_count(tu_fifo_t* f) +{ + return _tu_fifo_count(f, f->wr_idx, f->rd_idx); +} + +/******************************************************************************/ +/*! + @brief Check if FIFO is empty. + + As this function only reads the read and write pointers once, this function is + reentrant and thus thread and ISR save without any mutexes. + + @param[in] f + Pointer to the FIFO buffer to manipulate + + @returns Number of items in FIFO + */ +/******************************************************************************/ +bool tu_fifo_empty(tu_fifo_t* f) +{ + return _tu_fifo_empty(f->wr_idx, f->rd_idx); +} + +/******************************************************************************/ +/*! + @brief Check if FIFO is full. + + As this function only reads the read and write pointers once, this function is + reentrant and thus thread and ISR save without any mutexes. + + @param[in] f + Pointer to the FIFO buffer to manipulate + + @returns Number of items in FIFO + */ +/******************************************************************************/ +bool tu_fifo_full(tu_fifo_t* f) +{ + return _tu_fifo_full(f, f->wr_idx, f->rd_idx); +} + +/******************************************************************************/ +/*! + @brief Get remaining space in FIFO. + + As this function only reads the read and write pointers once, this function is + reentrant and thus thread and ISR save without any mutexes. + + @param[in] f + Pointer to the FIFO buffer to manipulate + + @returns Number of items in FIFO + */ +/******************************************************************************/ +uint16_t tu_fifo_remaining(tu_fifo_t* f) +{ + return _tu_fifo_remaining(f, f->wr_idx, f->rd_idx); +} + +/******************************************************************************/ +/*! + @brief Check if overflow happened. + + BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" + EXAMPLE with buffer depth: 100 + Maximum index space: (2^16)-1) - ((2^16)-1) % depth = 65500 + If you produce 65500 / 100 = 655 buffer overflows, the write pointer will overflow as well and + the check _tu_fifo_overflow() will not give you a valid result! Avoid such nasty things! + Use tu_fifo_correct_read_pointer() if overflow happened to set read pointer to correct index + for reading latest items! + + @param[in] f + Pointer to the FIFO buffer to manipulate + + @returns True if overflow happened + */ +/******************************************************************************/ +bool tu_fifo_overflow(tu_fifo_t* f) +{ + return _tu_fifo_overflow(f, f->wr_idx, f->rd_idx); +} + +// Only use in case tu_fifo_overflow() returned true! +void tu_fifo_correct_read_pointer(tu_fifo_t* f) +{ + _tu_fifo_correct_read_pointer(f, f->wr_idx); +} + +/******************************************************************************/ +/*! + @brief Read one element out of the buffer. This function will return the element located at the array index of the read pointer, and then increment the read pointer index. If the read @@ -120,19 +358,22 @@ static inline void _ff_push(tu_fifo_t* f, void const * data, uint16_t n) Pointer to the place holder for data read from the buffer @returns TRUE if the queue is not empty -*/ + */ /******************************************************************************/ bool tu_fifo_read(tu_fifo_t* f, void * buffer) { - if( tu_fifo_empty(f) ) return false; + tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! - tu_fifo_lock(f); + uint16_t r = f->rd_idx; - _ff_pull(f, buffer, 1); + // Peek the data + bool ret = _tu_fifo_peek_at(f, 0, buffer, f->wr_idx, r); + + // Advance pointer + f->rd_idx = advance_pointer(f, r, ret); tu_fifo_unlock(f); - - return true; + return ret; } /******************************************************************************/ @@ -149,35 +390,21 @@ bool tu_fifo_read(tu_fifo_t* f, void * buffer) Number of element that buffer can afford @returns number of items read from the FIFO -*/ + */ /******************************************************************************/ -uint16_t tu_fifo_read_n (tu_fifo_t* f, void * buffer, uint16_t count) +uint16_t tu_fifo_read_n(tu_fifo_t* f, void * buffer, uint16_t count) { - if(tu_fifo_empty(f)) return 0; + tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! - tu_fifo_lock(f); + uint16_t r = f->rd_idx; - // Limit up to fifo's count - if(count > f->count) count = f->count; + // Peek the data + count = _tu_fifo_peek_at_n(f, 0, buffer, count, f->wr_idx, r); - if(count + f->rd_idx <= f->depth) - { - _ff_pull(f, buffer, count); - } - else - { - uint16_t const part1 = f->depth - f->rd_idx; - - // Part 1: from rd_idx to end - _ff_pull(f, buffer, part1); - buffer = ((uint8_t*) buffer) + part1*f->item_size; - - // Part 2: start to remaining - _ff_pull(f, buffer, count-part1); - } + // Advance read pointer + f->rd_idx = advance_pointer(f, r, count); tu_fifo_unlock(f); - return count; } @@ -193,28 +420,37 @@ uint16_t tu_fifo_read_n (tu_fifo_t* f, void * buffer, uint16_t count) Pointer to the place holder for data read from the buffer @returns TRUE if the queue is not empty -*/ + */ /******************************************************************************/ bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer) { - if ( pos >= f->count ) return false; - - tu_fifo_lock(f); - - // rd_idx is pos=0 - uint16_t index = _ff_mod(f->rd_idx + pos, f->depth); - memcpy(p_buffer, - f->buffer + (index * f->item_size), - f->item_size); - - tu_fifo_unlock(f); - - return true; + return _tu_fifo_peek_at(f, pos, p_buffer, f->wr_idx, f->rd_idx); } /******************************************************************************/ /*! - @brief Write one element into the RX buffer. + @brief Read n items without removing it from the FIFO + + @param[in] f + Pointer to the FIFO buffer to manipulate + @param[in] pos + Position to read from in the FIFO buffer + @param[in] p_buffer + Pointer to the place holder for data read from the buffer + @param[in] n + Number of items to peek + + @returns Number of bytes written to p_buffer + */ +/******************************************************************************/ +uint16_t tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n) +{ + return _tu_fifo_peek_at_n(f, pos, p_buffer, n, f->wr_idx, f->rd_idx); +} + +/******************************************************************************/ +/*! + @brief Write one element into the buffer. This function will write one element into the array index specified by the write pointer and increment the write index. If the write index @@ -227,15 +463,23 @@ bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer) @returns TRUE if the data was written to the FIFO (overwrittable FIFO will always return TRUE) -*/ + */ /******************************************************************************/ -bool tu_fifo_write (tu_fifo_t* f, const void * data) +bool tu_fifo_write(tu_fifo_t* f, const void * data) { - if ( tu_fifo_full(f) && !f->overwritable ) return false; - tu_fifo_lock(f); - _ff_push(f, data, 1); + uint16_t w = f->wr_idx; + + if ( _tu_fifo_full(f, w, f->rd_idx) && !f->overwritable ) return false; + + uint16_t wRel = get_relative_pointer(f, w, 0); + + // Write data + _ff_push(f, data, wRel); + + // Advance pointer + f->wr_idx = advance_pointer(f, w, 1); tu_fifo_unlock(f); @@ -255,47 +499,42 @@ bool tu_fifo_write (tu_fifo_t* f, const void * data) @param[in] count Number of element @return Number of written elements -*/ + */ /******************************************************************************/ -uint16_t tu_fifo_write_n (tu_fifo_t* f, const void * data, uint16_t count) +uint16_t tu_fifo_write_n(tu_fifo_t* f, const void * data, uint16_t count) { if ( count == 0 ) return 0; tu_fifo_lock(f); + uint16_t w = f->wr_idx, r = f->rd_idx; uint8_t const* buf8 = (uint8_t const*) data; if (!f->overwritable) { // Not overwritable limit up to full - count = tu_min16(count, tu_fifo_remaining(f)); + count = tu_min16(count, _tu_fifo_remaining(f, w, r)); } else if (count > f->depth) { // Only copy last part buf8 = buf8 + (count - f->depth) * f->item_size; count = f->depth; - f->wr_idx = 0; - f->rd_idx = 0; - f->count = 0; + + // We start writing at the read pointer's position since we fill the complete + // buffer and we do not want to modify the read pointer within a write function! + // This would end up in a race condition with read functions! + f->wr_idx = r; } - if (count + f->wr_idx <= f->depth ) - { - _ff_push(f, buf8, count); - } - else - { - uint16_t const part1 = f->depth - f->wr_idx; + uint16_t wRel = get_relative_pointer(f, w, 0); - // Part 1: from wr_idx to end - _ff_push(f, buf8, part1); - buf8 += part1*f->item_size; + // Write data + _ff_push_n(f, buf8, count, wRel); + + // Advance pointer + f->wr_idx = advance_pointer(f, w, count); - // Part 2: start to remaining - _ff_push(f, buf8, count-part1); - } - tu_fifo_unlock(f); return count; @@ -307,13 +546,13 @@ uint16_t tu_fifo_write_n (tu_fifo_t* f, const void * data, uint16_t count) @param[in] f Pointer to the FIFO buffer to manipulate -*/ + */ /******************************************************************************/ bool tu_fifo_clear(tu_fifo_t *f) { tu_fifo_lock(f); - f->rd_idx = f->wr_idx = f->count = 0; + f->rd_idx = f->wr_idx = 0; tu_fifo_unlock(f); diff --git a/src/common/tusb_fifo.h b/src/common/tusb_fifo.h index fb0c896f..56491c37 100644 --- a/src/common/tusb_fifo.h +++ b/src/common/tusb_fifo.h @@ -52,14 +52,16 @@ */ typedef struct { - uint8_t* buffer ; ///< buffer pointer - uint16_t depth ; ///< max items - uint16_t item_size ; ///< size of each item - bool overwritable ; + uint8_t* buffer ; ///< buffer pointer + uint16_t depth ; ///< max items + uint16_t item_size ; ///< size of each item + bool overwritable ; - volatile uint16_t count ; ///< number of items in queue - volatile uint16_t wr_idx ; ///< write pointer - volatile uint16_t rd_idx ; ///< read pointer + uint16_t non_used_index_space ; ///< required for non-power-of-two buffer length + uint16_t max_pointer_idx ; ///< maximum absolute pointer index + + volatile uint16_t wr_idx ; ///< write pointer + volatile uint16_t rd_idx ; ///< read pointer #if CFG_FIFO_MUTEX tu_fifo_mutex_t mutex; @@ -67,13 +69,15 @@ typedef struct } tu_fifo_t; -#define TU_FIFO_DEF(_name, _depth, _type, _overwritable) \ - uint8_t _name##_buf[_depth*sizeof(_type)]; \ - tu_fifo_t _name = { \ - .buffer = _name##_buf, \ - .depth = _depth, \ - .item_size = sizeof(_type), \ - .overwritable = _overwritable, \ +#define TU_FIFO_DEF(_name, _depth, _type, _overwritable) \ + uint8_t _name##_buf[_depth*sizeof(_type)]; \ + tu_fifo_t _name = { \ + .buffer = _name##_buf, \ + .depth = _depth, \ + .item_size = sizeof(_type), \ + .overwritable = _overwritable, \ + .non_used_index_space = (2^16-1) % _depth, \ + .max_pointer_idx = (2^16-1) - ((2^16-1) % _depth), \ } bool tu_fifo_clear(tu_fifo_t *f); @@ -86,39 +90,28 @@ static inline void tu_fifo_config_mutex(tu_fifo_t *f, tu_fifo_mutex_t mutex_hdl) } #endif -bool tu_fifo_write (tu_fifo_t* f, void const * p_data); -uint16_t tu_fifo_write_n (tu_fifo_t* f, void const * p_data, uint16_t count); +bool tu_fifo_write (tu_fifo_t* f, void const * p_data); +uint16_t tu_fifo_write_n (tu_fifo_t* f, void const * p_data, uint16_t count); -bool tu_fifo_read (tu_fifo_t* f, void * p_buffer); -uint16_t tu_fifo_read_n (tu_fifo_t* f, void * p_buffer, uint16_t count); +bool tu_fifo_read (tu_fifo_t* f, void * p_buffer); +uint16_t tu_fifo_read_n (tu_fifo_t* f, void * p_buffer, uint16_t count); -bool tu_fifo_peek_at (tu_fifo_t* f, uint16_t pos, void * p_buffer); +bool tu_fifo_peek_at (tu_fifo_t* f, uint16_t pos, void * p_buffer); +uint16_t tu_fifo_peek_at_n (tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n); + +uint16_t tu_fifo_count (tu_fifo_t* f); +void tu_fifo_correct_read_pointer (tu_fifo_t* f); +bool tu_fifo_empty (tu_fifo_t* f); +bool tu_fifo_full (tu_fifo_t* f); +uint16_t tu_fifo_remaining (tu_fifo_t* f); +bool tu_fifo_overflow (tu_fifo_t* f); +void tu_fifo_correct_read_pointer (tu_fifo_t* f); static inline bool tu_fifo_peek(tu_fifo_t* f, void * p_buffer) { return tu_fifo_peek_at(f, 0, p_buffer); } -static inline bool tu_fifo_empty(tu_fifo_t* f) -{ - return (f->count == 0); -} - -static inline bool tu_fifo_full(tu_fifo_t* f) -{ - return (f->count == f->depth); -} - -static inline uint16_t tu_fifo_count(tu_fifo_t* f) -{ - return f->count; -} - -static inline uint16_t tu_fifo_remaining(tu_fifo_t* f) -{ - return f->depth - f->count; -} - static inline uint16_t tu_fifo_depth(tu_fifo_t* f) { return f->depth; From 9dfb78e9d82d0386de555aa291242ddfe835cd18 Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Tue, 15 Sep 2020 20:40:41 +0200 Subject: [PATCH 3/8] Tested, working. --- src/common/tusb_fifo.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index 4e274003..e7a2b469 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -65,8 +65,8 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si f->item_size = item_size; f->overwritable = overwritable; - f->non_used_index_space = ((2^16)-1) % depth; - f->max_pointer_idx = ((2^16)-1) - f->non_used_index_space; + f->non_used_index_space = 0x10000 % depth; + f->max_pointer_idx = 0xFFFF - f->non_used_index_space; f->rd_idx = f->wr_idx = 0; @@ -78,13 +78,14 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si // TODO: To be changed!! static inline uint16_t _ff_mod(uint16_t idx, uint16_t depth) { - return (idx < depth) ? idx : (idx-depth); +// return (idx < depth) ? idx : (idx-depth); + return idx % depth; } // send one item to FIFO WITHOUT updating write pointer -static inline void _ff_push(tu_fifo_t* f, void const * data, uint16_t n, uint16_t wRel) +static inline void _ff_push(tu_fifo_t* f, void const * data, uint16_t wRel) { - memcpy(f->buffer + (wRel * f->item_size), data, n*f->item_size); + memcpy(f->buffer + (wRel * f->item_size), data, f->item_size); } // send n items to FIFO WITHOUT updating write pointer @@ -107,13 +108,13 @@ static void _ff_push_n(tu_fifo_t* f, void const * data, uint16_t n, uint16_t wRe } // get one item from FIFO WITHOUT updating write pointer -static inline void _ff_pull(tu_fifo_t* f, void const * p_buffer, uint16_t rRel) +static inline void _ff_pull(tu_fifo_t* f, void * p_buffer, uint16_t rRel) { memcpy(p_buffer, f->buffer + (rRel * f->item_size), f->item_size); } // get n items from FIFO WITHOUT updating write pointer -static void _ff_pull_n(tu_fifo_t* f, void const * p_buffer, uint16_t n, uint16_t rRel) +static void _ff_pull_n(tu_fifo_t* f, void * p_buffer, uint16_t n, uint16_t rRel) { if(rRel + n <= f->depth) // Linear mode only { @@ -136,7 +137,7 @@ static uint16_t advance_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) { // We limit the index space of p such that a correct wrap around happens // Check for a wrap around or if we are in unused index space - This has to be checked first!! We are exploiting the wrap around to the correct index - if ((p > p + pos) || (p + pos >= f->max_pointer_idx)) + if ((p > p + pos) || (p + pos > f->max_pointer_idx)) { p = (p + pos) + f->non_used_index_space; } From 21299f90faf501422706ce36b6d3bb7c55bb3ecb Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Sat, 19 Sep 2020 11:46:43 +0200 Subject: [PATCH 4/8] Final cleanup. --- src/common/tusb_fifo.c | 148 ++++++++++++++++++++++++++++++++--------- src/common/tusb_fifo.h | 19 +++++- 2 files changed, 131 insertions(+), 36 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index e7a2b469..b83dfee4 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -2,7 +2,7 @@ * The MIT License (MIT) * * Copyright (c) 2019 Ha Thach (tinyusb.org) - * Copyright (c) 2020 Reinhard Panhuber + * Copyright (c) 2020 Reinhard Panhuber - rework to unmasked pointers * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -75,10 +75,10 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si return true; } -// TODO: To be changed!! +// Static functions are intended to work on local variables + static inline uint16_t _ff_mod(uint16_t idx, uint16_t depth) { -// return (idx < depth) ? idx : (idx-depth); return idx % depth; } @@ -148,6 +148,22 @@ static uint16_t advance_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) return p; } +// Backward an absolute pointer +static uint16_t backward_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) +{ + // We limit the index space of p such that a correct wrap around happens + // Check for a wrap around or if we are in unused index space - This has to be checked first!! We are exploiting the wrap around to the correct index + if ((p < p - pos) || (p - pos > f->max_pointer_idx)) + { + p = (p - pos) - f->non_used_index_space; + } + else + { + p -= pos; + } + return p; +} + // get relative from absolute pointer static uint16_t get_relative_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) { @@ -178,13 +194,15 @@ static inline bool _tu_fifo_full(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) } // Works on local copies of w and r -// BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" -// EXAMPLE with buffer depth: 100 -// Maximum index space: (2^16)-1) - ((2^16)-1) % depth = 65500 -// If you produce 65500 / 100 = 655 buffer overflows, the write pointer will overflow as well and -// the check _tu_fifo_overflow() will not give you a valid result! Avoid such nasty things! -// Use _tu_fifo_correct_read_pointer() if overflow happened to set read pointer to correct index -// for reading latest items! +//BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" +//EXAMPLE with buffer depth: 100 +//Maximum index space: (2^16) - (2^16) % depth = 65500 +//If you produce 65500 / 100 + 1 = 656 buffer overflows, the write pointer will overflow as well and +//the check _tu_fifo_overflow() will not give you a valid result! Avoid such nasty things! +//All reading functions (read, peek) check for overflows and correct read pointer on their own such +//that latest items are read. +//If required (e.g. for DMA use) you can also correct the read pointer by +//tu_fifo_correct_read_pointer(). static inline bool _tu_fifo_overflow(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) { return (_tu_fifo_count(f, wAbs, rAbs) > f->depth); @@ -194,16 +212,22 @@ static inline bool _tu_fifo_overflow(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) // For more details see _tu_fifo_overflow()! static inline void _tu_fifo_correct_read_pointer(tu_fifo_t* f, uint16_t wAbs) { - tu_fifo_lock(f); - f->rd_idx = advance_pointer(f, f->wr_idx, 1); - tu_fifo_unlock(f); + f->rd_idx = backward_pointer(f, wAbs, f->depth); } // Works on local copies of w and r +// Must be protected by mutexes since in case of an overflow read pointer gets modified static bool _tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t wAbs, uint16_t rAbs) { uint16_t cnt = _tu_fifo_count(f, wAbs, rAbs); + // Check overflow and correct if required + if (cnt > f->depth) + { + _tu_fifo_correct_read_pointer(f, wAbs); + cnt = f->depth; + } + // Skip beginning of buffer if (cnt == 0 || pos >= cnt) return false; @@ -216,10 +240,18 @@ static bool _tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16 } // Works on local copies of w and r +// Must be protected by mutexes since in case of an overflow read pointer gets modified static uint16_t _tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n, uint16_t wAbs, uint16_t rAbs) { uint16_t cnt = _tu_fifo_count(f, wAbs, rAbs); + // Check overflow and correct if required + if (cnt > f->depth) + { + _tu_fifo_correct_read_pointer(f, wAbs); + cnt = f->depth; + } + // Skip beginning of buffer if (cnt == 0 || pos >= cnt) return 0; @@ -322,11 +354,13 @@ uint16_t tu_fifo_remaining(tu_fifo_t* f) BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" EXAMPLE with buffer depth: 100 - Maximum index space: (2^16)-1) - ((2^16)-1) % depth = 65500 - If you produce 65500 / 100 = 655 buffer overflows, the write pointer will overflow as well and + Maximum index space: (2^16) - (2^16) % depth = 65500 + If you produce 65500 / 100 + 1 = 656 buffer overflows, the write pointer will overflow as well and the check _tu_fifo_overflow() will not give you a valid result! Avoid such nasty things! - Use tu_fifo_correct_read_pointer() if overflow happened to set read pointer to correct index - for reading latest items! + All reading functions (read, peek) check for overflows and correct read pointer on their own such + that latest items are read. + If required (e.g. for DMA use) you can also correct the read pointer by + tu_fifo_correct_read_pointer(). @param[in] f Pointer to the FIFO buffer to manipulate @@ -342,7 +376,9 @@ bool tu_fifo_overflow(tu_fifo_t* f) // Only use in case tu_fifo_overflow() returned true! void tu_fifo_correct_read_pointer(tu_fifo_t* f) { + tu_fifo_lock(f); _tu_fifo_correct_read_pointer(f, f->wr_idx); + tu_fifo_unlock(f); } /******************************************************************************/ @@ -350,8 +386,8 @@ void tu_fifo_correct_read_pointer(tu_fifo_t* f) @brief Read one element out of the buffer. This function will return the element located at the array index of the - read pointer, and then increment the read pointer index. If the read - pointer exceeds the maximum buffer size, it will roll over to zero. + read pointer, and then increment the read pointer index. + This function checks for an overflow and corrects read pointer if required. @param[in] f Pointer to the FIFO buffer to manipulate @@ -380,8 +416,8 @@ bool tu_fifo_read(tu_fifo_t* f, void * buffer) /******************************************************************************/ /*! @brief This function will read n elements from the array index specified by - the read pointer and increment the read index. If the read index - exceeds the max buffer size, then it will roll over to zero. + the read pointer and increment the read index. + This function checks for an overflow and corrects read pointer if required. @param[in] f Pointer to the FIFO buffer to manipulate @@ -411,12 +447,13 @@ uint16_t tu_fifo_read_n(tu_fifo_t* f, void * buffer, uint16_t count) /******************************************************************************/ /*! - @brief Read one item without removing it from the FIFO + @brief Read one item without removing it from the FIFO. + This function checks for an overflow and corrects read pointer if required. @param[in] f Pointer to the FIFO buffer to manipulate @param[in] pos - Position to read from in the FIFO buffer + Position to read from in the FIFO buffer with respect to read pointer @param[in] p_buffer Pointer to the place holder for data read from the buffer @@ -425,17 +462,21 @@ uint16_t tu_fifo_read_n(tu_fifo_t* f, void * buffer, uint16_t count) /******************************************************************************/ bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer) { - return _tu_fifo_peek_at(f, pos, p_buffer, f->wr_idx, f->rd_idx); + tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! + bool ret = _tu_fifo_peek_at(f, pos, p_buffer, f->wr_idx, f->rd_idx); + tu_fifo_unlock(f); + return ret; } /******************************************************************************/ /*! @brief Read n items without removing it from the FIFO + This function checks for an overflow and corrects read pointer if required. @param[in] f Pointer to the FIFO buffer to manipulate @param[in] pos - Position to read from in the FIFO buffer + Position to read from in the FIFO buffer with respect to read pointer @param[in] p_buffer Pointer to the place holder for data read from the buffer @param[in] n @@ -446,7 +487,10 @@ bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer) /******************************************************************************/ uint16_t tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n) { - return _tu_fifo_peek_at_n(f, pos, p_buffer, n, f->wr_idx, f->rd_idx); + tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! + bool ret = _tu_fifo_peek_at_n(f, pos, p_buffer, n, f->wr_idx, f->rd_idx); + tu_fifo_unlock(f); + return ret; } /******************************************************************************/ @@ -454,8 +498,7 @@ uint16_t tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t @brief Write one element into the buffer. This function will write one element into the array index specified by - the write pointer and increment the write index. If the write index - exceeds the max buffer size, then it will roll over to zero. + the write pointer and increment the write index. @param[in] f Pointer to the FIFO buffer to manipulate @@ -490,8 +533,7 @@ bool tu_fifo_write(tu_fifo_t* f, const void * data) /******************************************************************************/ /*! @brief This function will write n elements into the array index specified by - the write pointer and increment the write index. If the write index - exceeds the max buffer size, then it will roll over to zero. + the write pointer and increment the write index. @param[in] f Pointer to the FIFO buffer to manipulate @@ -543,7 +585,7 @@ uint16_t tu_fifo_write_n(tu_fifo_t* f, const void * data, uint16_t count) /******************************************************************************/ /*! - @brief Clear the fifo read and write pointers and set length to zero + @brief Clear the fifo read and write pointers @param[in] f Pointer to the FIFO buffer to manipulate @@ -552,10 +594,50 @@ uint16_t tu_fifo_write_n(tu_fifo_t* f, const void * data, uint16_t count) bool tu_fifo_clear(tu_fifo_t *f) { tu_fifo_lock(f); - f->rd_idx = f->wr_idx = 0; - tu_fifo_unlock(f); return true; } + +/******************************************************************************/ +/*! + @brief Advance write pointer - intended to be used in combination with DMA. + It is possible to fill the FIFO by use of a DMA in circular mode. Within + DMA ISRs you may update the write pointer to be able to read from the FIFO. + As long as the DMA is the only process writing into the FIFO this is safe + to use. + + USE WITH CARE - WE DO NOT CONDUCT SAFTY CHECKS HERE! + + @param[in] f + Pointer to the FIFO buffer to manipulate + @param[in] n + Number of items the write pointer moves forward + */ +/******************************************************************************/ +void tu_fifo_advance_write_pointer(tu_fifo_t *f, uint16_t n) +{ + f->wr_idx = advance_pointer(f, f->wr_idx, n); +} + +/******************************************************************************/ +/*! + @brief Advance read pointer - intended to be used in combination with DMA. + It is possible to read from the FIFO by use of a DMA in linear mode. Within + DMA ISRs you may update the read pointer to be able to again write into the + FIFO. As long as the DMA is the only process reading from the FIFO this is + safe to use. + + USE WITH CARE - WE DO NOT CONDUCT SAFTY CHECKS HERE! + + @param[in] f + Pointer to the FIFO buffer to manipulate + @param[in] n + Number of items the read pointer moves forward + */ +/******************************************************************************/ +void tu_fifo_advance_read_pointer(tu_fifo_t *f, uint16_t n) +{ + f->rd_idx = advance_pointer(f, f->rd_idx, n); +} diff --git a/src/common/tusb_fifo.h b/src/common/tusb_fifo.h index 56491c37..66888ef1 100644 --- a/src/common/tusb_fifo.h +++ b/src/common/tusb_fifo.h @@ -31,6 +31,15 @@ #ifndef _TUSB_FIFO_H_ #define _TUSB_FIFO_H_ +// Due to the use of unmasked pointers, this FIFO does not suffer from loosing +// one item slice. Furthermore, write and read operations are completely +// decoupled as write and read functions do not modify a common state. Henceforth, +// writing or reading from the FIFO within an ISR is safe as long as no other +// process (thread or ISR) interferes. +// Also, this FIFO is ready to be used in combination with a DMA as the write and +// read pointers can be updated from within a DMA ISR. Overflows are detectable +// within a certain number (see tu_fifo_overflow()). + // mutex is only needed for RTOS // for OS None, we don't get preempted #define CFG_FIFO_MUTEX (CFG_TUSB_OS != OPT_OS_NONE) @@ -76,8 +85,8 @@ typedef struct .depth = _depth, \ .item_size = sizeof(_type), \ .overwritable = _overwritable, \ - .non_used_index_space = (2^16-1) % _depth, \ - .max_pointer_idx = (2^16-1) - ((2^16-1) % _depth), \ + .non_used_index_space = 0x10000 % _depth, \ + .max_pointer_idx = 0xFFFF - (0x10000 % _depth), \ } bool tu_fifo_clear(tu_fifo_t *f); @@ -100,13 +109,17 @@ bool tu_fifo_peek_at (tu_fifo_t* f, uint16_t pos, void * p_bu uint16_t tu_fifo_peek_at_n (tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n); uint16_t tu_fifo_count (tu_fifo_t* f); -void tu_fifo_correct_read_pointer (tu_fifo_t* f); bool tu_fifo_empty (tu_fifo_t* f); bool tu_fifo_full (tu_fifo_t* f); uint16_t tu_fifo_remaining (tu_fifo_t* f); bool tu_fifo_overflow (tu_fifo_t* f); void tu_fifo_correct_read_pointer (tu_fifo_t* f); +// Pointer modifications intended to be used in combinations with DMAs. +// USE WITH CARE - NO SAFTY CHECKS CONDUCTED HERE! NOT MUTEX PROTECTED! +void tu_fifo_advance_write_pointer (tu_fifo_t *f, uint16_t n); +void tu_fifo_advance_read_pointer (tu_fifo_t *f, uint16_t n); + static inline bool tu_fifo_peek(tu_fifo_t* f, void * p_buffer) { return tu_fifo_peek_at(f, 0, p_buffer); From 9bdeafb29503b65d2c46e71a4c7e80b753bad55a Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Wed, 23 Sep 2020 20:48:03 +0200 Subject: [PATCH 5/8] Change maximum depth to 2^15 which allows for a fast modulo substitute. Thus, however, overflows are detectable only for one time FIFO depth. --- src/common/tusb_fifo.c | 37 +++++++++++++++++-------------------- src/common/tusb_fifo.h | 40 ++++++++++++++++++++-------------------- 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index b83dfee4..84fc9bb8 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -58,6 +58,8 @@ static void tu_fifo_unlock(tu_fifo_t *f) bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_size, bool overwritable) { + if (depth > 0x8000) return false; // Maximum depth is 2^15 items + tu_fifo_lock(f); f->buffer = (uint8_t*) buffer; @@ -65,8 +67,8 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si f->item_size = item_size; f->overwritable = overwritable; - f->non_used_index_space = 0x10000 % depth; - f->max_pointer_idx = 0xFFFF - f->non_used_index_space; + f->max_pointer_idx = 2*depth - 1; // Limit index space to 2*depth - this allows for a fast "modulo" calculation but limits the maximum depth to 2^16/2 = 2^15 and buffer overflows are detectable only if overflow happens once (important for unsupervised DMA applications) + f->non_used_index_space = 0xFFFF - f->max_pointer_idx; f->rd_idx = f->wr_idx = 0; @@ -79,7 +81,9 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si static inline uint16_t _ff_mod(uint16_t idx, uint16_t depth) { - return idx % depth; +// return idx % depth; + idx -= depth & -(idx > depth); + return idx -= depth & -(idx > depth); } // send one item to FIFO WITHOUT updating write pointer @@ -194,15 +198,11 @@ static inline bool _tu_fifo_full(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) } // Works on local copies of w and r -//BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" -//EXAMPLE with buffer depth: 100 -//Maximum index space: (2^16) - (2^16) % depth = 65500 -//If you produce 65500 / 100 + 1 = 656 buffer overflows, the write pointer will overflow as well and -//the check _tu_fifo_overflow() will not give you a valid result! Avoid such nasty things! -//All reading functions (read, peek) check for overflows and correct read pointer on their own such -//that latest items are read. -//If required (e.g. for DMA use) you can also correct the read pointer by -//tu_fifo_correct_read_pointer(). +// BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" +// Only one overflow is allowed for this function to work e.g. if depth = 100, you must not +// write more than 2*depth-1 items in one rush without updating write pointer. Otherwise +// write pointer wraps and you pointer states are messed up. This can only happen if you +// use DMAs, write functions do not allow such an error. static inline bool _tu_fifo_overflow(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) { return (_tu_fifo_count(f, wAbs, rAbs) > f->depth); @@ -249,6 +249,7 @@ static uint16_t _tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, if (cnt > f->depth) { _tu_fifo_correct_read_pointer(f, wAbs); + rAbs = f->rd_idx; cnt = f->depth; } @@ -401,13 +402,11 @@ bool tu_fifo_read(tu_fifo_t* f, void * buffer) { tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! - uint16_t r = f->rd_idx; - // Peek the data - bool ret = _tu_fifo_peek_at(f, 0, buffer, f->wr_idx, r); + bool ret = _tu_fifo_peek_at(f, 0, buffer, f->wr_idx, f->rd_idx); // f->rd_idx might get modified in case of an overflow so we can not use a local variable // Advance pointer - f->rd_idx = advance_pointer(f, r, ret); + f->rd_idx = advance_pointer(f, f->rd_idx, ret); tu_fifo_unlock(f); return ret; @@ -433,13 +432,11 @@ uint16_t tu_fifo_read_n(tu_fifo_t* f, void * buffer, uint16_t count) { tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! - uint16_t r = f->rd_idx; - // Peek the data - count = _tu_fifo_peek_at_n(f, 0, buffer, count, f->wr_idx, r); + count = _tu_fifo_peek_at_n(f, 0, buffer, count, f->wr_idx, f->rd_idx); // f->rd_idx might get modified in case of an overflow so we can not use a local variable // Advance read pointer - f->rd_idx = advance_pointer(f, r, count); + f->rd_idx = advance_pointer(f, f->rd_idx, count); tu_fifo_unlock(f); return count; diff --git a/src/common/tusb_fifo.h b/src/common/tusb_fifo.h index 66888ef1..23f5d844 100644 --- a/src/common/tusb_fifo.h +++ b/src/common/tusb_fifo.h @@ -48,7 +48,7 @@ #include #ifdef __cplusplus - extern "C" { +extern "C" { #endif #if CFG_FIFO_MUTEX @@ -61,16 +61,16 @@ */ typedef struct { - uint8_t* buffer ; ///< buffer pointer - uint16_t depth ; ///< max items - uint16_t item_size ; ///< size of each item - bool overwritable ; + uint8_t* buffer ; ///< buffer pointer + uint16_t depth ; ///< max items + uint16_t item_size ; ///< size of each item + bool overwritable ; - uint16_t non_used_index_space ; ///< required for non-power-of-two buffer length - uint16_t max_pointer_idx ; ///< maximum absolute pointer index + uint16_t non_used_index_space ; ///< required for non-power-of-two buffer length + uint16_t max_pointer_idx ; ///< maximum absolute pointer index - volatile uint16_t wr_idx ; ///< write pointer - volatile uint16_t rd_idx ; ///< read pointer + volatile uint16_t wr_idx ; ///< write pointer + volatile uint16_t rd_idx ; ///< read pointer #if CFG_FIFO_MUTEX tu_fifo_mutex_t mutex; @@ -78,16 +78,16 @@ typedef struct } tu_fifo_t; -#define TU_FIFO_DEF(_name, _depth, _type, _overwritable) \ - uint8_t _name##_buf[_depth*sizeof(_type)]; \ - tu_fifo_t _name = { \ - .buffer = _name##_buf, \ - .depth = _depth, \ - .item_size = sizeof(_type), \ - .overwritable = _overwritable, \ - .non_used_index_space = 0x10000 % _depth, \ - .max_pointer_idx = 0xFFFF - (0x10000 % _depth), \ - } +#define TU_FIFO_DEF(_name, _depth, _type, _overwritable) \ + uint8_t _name##_buf[_depth*sizeof(_type)]; \ + tu_fifo_t _name = { \ + .buffer = _name##_buf, \ + .depth = _depth, \ + .item_size = sizeof(_type), \ + .overwritable = _overwritable, \ + .max_pointer_idx = 2*_depth-1, \ + .non_used_index_space = 0xFFFF - 2*_depth-1, \ + } bool tu_fifo_clear(tu_fifo_t *f); bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_size, bool overwritable); @@ -131,7 +131,7 @@ static inline uint16_t tu_fifo_depth(tu_fifo_t* f) } #ifdef __cplusplus - } +} #endif #endif /* _TUSB_FIFO_H_ */ From 52c9a467b42c4448251753a295343496a9265b4f Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Wed, 23 Sep 2020 21:36:30 +0200 Subject: [PATCH 6/8] Fix bug in modulo substitute. --- src/common/tusb_fifo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index 84fc9bb8..cf4e6a51 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -82,8 +82,8 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si static inline uint16_t _ff_mod(uint16_t idx, uint16_t depth) { // return idx % depth; - idx -= depth & -(idx > depth); - return idx -= depth & -(idx > depth); + idx -= depth & -(idx >= depth); + return idx -= depth & -(idx >= depth); } // send one item to FIFO WITHOUT updating write pointer From 9ddcfc09d7c0f076248307f76222d5b1db9e0882 Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Fri, 25 Sep 2020 15:58:28 +0200 Subject: [PATCH 7/8] Fix wrong comments, rename pos to offset, rename overflow(). --- src/common/tusb_fifo.c | 69 +++++++++++++++++++++--------------------- src/common/tusb_fifo.h | 2 +- 2 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index cf4e6a51..1e0c6b5e 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -111,13 +111,13 @@ static void _ff_push_n(tu_fifo_t* f, void const * data, uint16_t n, uint16_t wRe } } -// get one item from FIFO WITHOUT updating write pointer +// get one item from FIFO WITHOUT updating read pointer static inline void _ff_pull(tu_fifo_t* f, void * p_buffer, uint16_t rRel) { memcpy(p_buffer, f->buffer + (rRel * f->item_size), f->item_size); } -// get n items from FIFO WITHOUT updating write pointer +// get n items from FIFO WITHOUT updating read pointer static void _ff_pull_n(tu_fifo_t* f, void * p_buffer, uint16_t n, uint16_t rRel) { if(rRel + n <= f->depth) // Linear mode only @@ -137,45 +137,45 @@ static void _ff_pull_n(tu_fifo_t* f, void * p_buffer, uint16_t n, uint16_t rRel) } // Advance an absolute pointer -static uint16_t advance_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) +static uint16_t advance_pointer(tu_fifo_t* f, uint16_t p, uint16_t offset) { // We limit the index space of p such that a correct wrap around happens // Check for a wrap around or if we are in unused index space - This has to be checked first!! We are exploiting the wrap around to the correct index - if ((p > p + pos) || (p + pos > f->max_pointer_idx)) + if ((p > p + offset) || (p + offset > f->max_pointer_idx)) { - p = (p + pos) + f->non_used_index_space; + p = (p + offset) + f->non_used_index_space; } else { - p += pos; + p += offset; } return p; } // Backward an absolute pointer -static uint16_t backward_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) +static uint16_t backward_pointer(tu_fifo_t* f, uint16_t p, uint16_t offset) { // We limit the index space of p such that a correct wrap around happens // Check for a wrap around or if we are in unused index space - This has to be checked first!! We are exploiting the wrap around to the correct index - if ((p < p - pos) || (p - pos > f->max_pointer_idx)) + if ((p < p - offset) || (p - offset > f->max_pointer_idx)) { - p = (p - pos) - f->non_used_index_space; + p = (p - offset) - f->non_used_index_space; } else { - p -= pos; + p -= offset; } return p; } // get relative from absolute pointer -static uint16_t get_relative_pointer(tu_fifo_t* f, uint16_t p, uint16_t pos) +static uint16_t get_relative_pointer(tu_fifo_t* f, uint16_t p, uint16_t offset) { - return _ff_mod(advance_pointer(f, p, pos), f->depth); + return _ff_mod(advance_pointer(f, p, offset), f->depth); } // Works on local copies of w and r -static uint16_t _tu_fifo_count(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) +static inline uint16_t _tu_fifo_count(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) { uint16_t cnt = wAbs-rAbs; @@ -203,7 +203,7 @@ static inline bool _tu_fifo_full(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) // write more than 2*depth-1 items in one rush without updating write pointer. Otherwise // write pointer wraps and you pointer states are messed up. This can only happen if you // use DMAs, write functions do not allow such an error. -static inline bool _tu_fifo_overflow(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) +static inline bool _tu_fifo_overflowed(tu_fifo_t* f, uint16_t wAbs, uint16_t rAbs) { return (_tu_fifo_count(f, wAbs, rAbs) > f->depth); } @@ -217,7 +217,7 @@ static inline void _tu_fifo_correct_read_pointer(tu_fifo_t* f, uint16_t wAbs) // Works on local copies of w and r // Must be protected by mutexes since in case of an overflow read pointer gets modified -static bool _tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t wAbs, uint16_t rAbs) +static bool _tu_fifo_peek_at(tu_fifo_t* f, uint16_t offset, void * p_buffer, uint16_t wAbs, uint16_t rAbs) { uint16_t cnt = _tu_fifo_count(f, wAbs, rAbs); @@ -229,9 +229,9 @@ static bool _tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16 } // Skip beginning of buffer - if (cnt == 0 || pos >= cnt) return false; + if (cnt == 0 || offset >= cnt) return false; - uint16_t rRel = get_relative_pointer(f, rAbs, pos); + uint16_t rRel = get_relative_pointer(f, rAbs, offset); // Peek data _ff_pull(f, p_buffer, rRel); @@ -241,7 +241,7 @@ static bool _tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16 // Works on local copies of w and r // Must be protected by mutexes since in case of an overflow read pointer gets modified -static uint16_t _tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n, uint16_t wAbs, uint16_t rAbs) +static uint16_t _tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t offset, void * p_buffer, uint16_t n, uint16_t wAbs, uint16_t rAbs) { uint16_t cnt = _tu_fifo_count(f, wAbs, rAbs); @@ -254,16 +254,16 @@ static uint16_t _tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, } // Skip beginning of buffer - if (cnt == 0 || pos >= cnt) return 0; + if (cnt == 0 || offset >= cnt) return 0; - // Check if we can read something at and after pos - if too less is available we read what remains - cnt -= pos; + // Check if we can read something at and after offset - if too less is available we read what remains + cnt -= offset; if (cnt < n) { if (cnt == 0) return 0; n = cnt; } - uint16_t rRel = get_relative_pointer(f, rAbs, pos); + uint16_t rRel = get_relative_pointer(f, rAbs, offset); // Peek data _ff_pull_n(f, p_buffer, n, rRel); @@ -354,10 +354,11 @@ uint16_t tu_fifo_remaining(tu_fifo_t* f) @brief Check if overflow happened. BE AWARE - THIS FUNCTION MIGHT NOT GIVE A CORRECT ANSWERE IN CASE WRITE POINTER "OVERFLOWS" - EXAMPLE with buffer depth: 100 - Maximum index space: (2^16) - (2^16) % depth = 65500 - If you produce 65500 / 100 + 1 = 656 buffer overflows, the write pointer will overflow as well and - the check _tu_fifo_overflow() will not give you a valid result! Avoid such nasty things! + Only one overflow is allowed for this function to work e.g. if depth = 100, you must not + write more than 2*depth-1 items in one rush without updating write pointer. Otherwise + write pointer wraps and you pointer states are messed up. This can only happen if you + use DMAs, write functions do not allow such an error. Avoid such nasty things! + All reading functions (read, peek) check for overflows and correct read pointer on their own such that latest items are read. If required (e.g. for DMA use) you can also correct the read pointer by @@ -369,9 +370,9 @@ uint16_t tu_fifo_remaining(tu_fifo_t* f) @returns True if overflow happened */ /******************************************************************************/ -bool tu_fifo_overflow(tu_fifo_t* f) +bool tu_fifo_overflowed(tu_fifo_t* f) { - return _tu_fifo_overflow(f, f->wr_idx, f->rd_idx); + return _tu_fifo_overflowed(f, f->wr_idx, f->rd_idx); } // Only use in case tu_fifo_overflow() returned true! @@ -449,7 +450,7 @@ uint16_t tu_fifo_read_n(tu_fifo_t* f, void * buffer, uint16_t count) @param[in] f Pointer to the FIFO buffer to manipulate - @param[in] pos + @param[in] offset Position to read from in the FIFO buffer with respect to read pointer @param[in] p_buffer Pointer to the place holder for data read from the buffer @@ -457,10 +458,10 @@ uint16_t tu_fifo_read_n(tu_fifo_t* f, void * buffer, uint16_t count) @returns TRUE if the queue is not empty */ /******************************************************************************/ -bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer) +bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t offset, void * p_buffer) { tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! - bool ret = _tu_fifo_peek_at(f, pos, p_buffer, f->wr_idx, f->rd_idx); + bool ret = _tu_fifo_peek_at(f, offset, p_buffer, f->wr_idx, f->rd_idx); tu_fifo_unlock(f); return ret; } @@ -472,7 +473,7 @@ bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer) @param[in] f Pointer to the FIFO buffer to manipulate - @param[in] pos + @param[in] offset Position to read from in the FIFO buffer with respect to read pointer @param[in] p_buffer Pointer to the place holder for data read from the buffer @@ -482,10 +483,10 @@ bool tu_fifo_peek_at(tu_fifo_t* f, uint16_t pos, void * p_buffer) @returns Number of bytes written to p_buffer */ /******************************************************************************/ -uint16_t tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t pos, void * p_buffer, uint16_t n) +uint16_t tu_fifo_peek_at_n(tu_fifo_t* f, uint16_t offset, void * p_buffer, uint16_t n) { tu_fifo_lock(f); // TODO: Here we may distinguish for read and write pointer mutexes! - bool ret = _tu_fifo_peek_at_n(f, pos, p_buffer, n, f->wr_idx, f->rd_idx); + bool ret = _tu_fifo_peek_at_n(f, offset, p_buffer, n, f->wr_idx, f->rd_idx); tu_fifo_unlock(f); return ret; } diff --git a/src/common/tusb_fifo.h b/src/common/tusb_fifo.h index 23f5d844..b8769574 100644 --- a/src/common/tusb_fifo.h +++ b/src/common/tusb_fifo.h @@ -112,7 +112,7 @@ uint16_t tu_fifo_count (tu_fifo_t* f); bool tu_fifo_empty (tu_fifo_t* f); bool tu_fifo_full (tu_fifo_t* f); uint16_t tu_fifo_remaining (tu_fifo_t* f); -bool tu_fifo_overflow (tu_fifo_t* f); +bool tu_fifo_overflowed (tu_fifo_t* f); void tu_fifo_correct_read_pointer (tu_fifo_t* f); // Pointer modifications intended to be used in combinations with DMAs. From 8dcb10493330513b5b017306634a936413f1ecd5 Mon Sep 17 00:00:00 2001 From: Reinhard Panhuber Date: Sat, 26 Sep 2020 11:00:31 +0200 Subject: [PATCH 8/8] Change modulo substitute to while ( idx >= depth) idx -= depth; --- src/common/tusb_fifo.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index 1e0c6b5e..f41cf9f6 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -81,9 +81,8 @@ bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_si static inline uint16_t _ff_mod(uint16_t idx, uint16_t depth) { -// return idx % depth; - idx -= depth & -(idx >= depth); - return idx -= depth & -(idx >= depth); + while ( idx >= depth) idx -= depth; + return idx; } // send one item to FIFO WITHOUT updating write pointer