Skip to content

Fix fifo overflow correction.#940

Merged
hathach merged 3 commits into
hathach:masterfrom
HiFiPhile:fifo_fix
Jun 30, 2021
Merged

Fix fifo overflow correction.#940
hathach merged 3 commits into
hathach:masterfrom
HiFiPhile:fifo_fix

Conversation

@HiFiPhile
Copy link
Copy Markdown
Collaborator

Describe the PR
Fix fifo overflow correction.

Additional context
If applicable, add any other context about the PR and/or screenshots here.

@HiFiPhile
Copy link
Copy Markdown
Collaborator Author

HiFiPhile commented Jun 30, 2021

For advance_pointer

  if ((p > p + offset) || (p + offset > f->max_pointer_idx))
  {
    p = (p + offset) + f->non_used_index_space;
  }
  else
  {
    p += offset;
  }

I feel like the check can be simplified into if (p + offset > f->max_pointer_idx)), have I missed something ?

@PanRe
Copy link
Copy Markdown
Collaborator

PanRe commented Jun 30, 2021

For advance_pointer

  if ((p > p + offset) || (p + offset > f->max_pointer_idx))
  {
    p = (p + offset) + f->non_used_index_space;
  }
  else
  {
    p += offset;
  }

I feel like the check can be simplified into if (p + offset > f->max_pointer_idx)), have I missed something ?

The first condition covers the case when p overflows solely by offset. It is possible that the second condition never hits i.e. if offset is big enough to "jump over" the non-used index space.

Comment thread src/common/tusb_fifo.c
@@ -325,7 +325,7 @@ static uint16_t advance_pointer(tu_fifo_t* f, uint16_t p, uint16_t offset)
// We are exploiting the wrap around to the correct index

// TODO warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GCC seem to be right about this. I got the warning when compiling with gnu++17, haven't got time to trace it down. So I put the warning comment here. Thanks for the PR.

Copy link
Copy Markdown
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superb! Thank you @HiFiPhile for fixing the issue and @PanRe for reviewing it. I will try to recompile with gcc++17 later on, and remove warning comment if it is indeed the case.

@hathach hathach merged commit 9b3ec69 into hathach:master Jun 30, 2021
@HiFiPhile HiFiPhile deleted the fifo_fix branch July 6, 2021 16:36
7FM pushed a commit to 7FM/tinyusb that referenced this pull request Aug 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants