Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Tx FIFO memory calculation #26

Merged
merged 2 commits into from
Mar 20, 2022

Conversation

dgoodlad
Copy link
Contributor

This used to allocate 8 * 16 words minimum, no matter how many Tx endpoints where active. This was because tx_fifo_size_words is set to [0; 9] in new. Now we only account for memory allocated to enabled Tx endpoints when determining how much has already been used by limiting the elements we consider to those before the current endpoint being allocated.

This used to allocate 9 * 16 words minimum, no matter how many tx
endpoints where active. Now we only account for memory allocated to
enabled Tx endpoints when determining how much has already been used.
@Disasm
Copy link
Member

Disasm commented Mar 15, 2022

No, this loop doesn't allocate anything, actual allocation happens below. In fact, initialization value should be [16; 9] because as mentioned in STM32F466 Reference (RM0390):

Bits 31:16 INEPTXFD[15:0]: IN endpoint Tx FIFO depth
            This value is in terms of 32-bit words.
            Minimum value is 16

@dgoodlad
Copy link
Contributor Author

It may not allocate anything, but it calculates the used memory incorrectly, which results in throwing a EndpointMemoryOverflow

@dgoodlad
Copy link
Contributor Author

RM0390, if I recall correctly (I'll look it up in a bit), says that only applies for active endpoints.

@Disasm
Copy link
Member

Disasm commented Mar 15, 2022

If this is true (I also think so, but I was unable to find any evidence in RMs), then I'd suggest changing it to something like

used += self.tx_fifo_size_words.iter().sum();

@dgoodlad
Copy link
Contributor Author

The best quote I can glean from the RM is

In peripheral mode an additional Tx FIFO is instructed for each active IN endpoint. Any FIFO size is software configured to better meet the application requirements.

which seems to imply that it's only for active endpoints.

Practically speaking, my application (an implementation of USB Audio Class 2.0) works fine with this change. It's got two endpoints, one large OUT endpoint for audio data and one 3-byte IN ep for the audio feedback channel.

Copy link
Member

@Disasm Disasm left a comment

Choose a reason for hiding this comment

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

Thank you!

@Disasm Disasm merged commit b53dbe7 into stm32-rs:master Mar 20, 2022
@dgoodlad dgoodlad mentioned this pull request Oct 6, 2022
dgoodlad added a commit to dgoodlad/stm32f4xx-hal that referenced this pull request Oct 7, 2022
Fixes an issue with FIFO allocation limits; see
stm32-rs/synopsys-usb-otg#26
dgoodlad added a commit to dgoodlad/stm32f4xx-hal that referenced this pull request Oct 7, 2022
Fixes an issue with FIFO allocation limits; see
stm32-rs/synopsys-usb-otg#26
bors bot added a commit to stm32-rs/stm32f4xx-hal that referenced this pull request Oct 7, 2022
535: Bump synopsys-usb-otg to 0.3.1 r=burrbull a=dgoodlad

Fixes an issue with FIFO allocation limits. tl;dr the previous version assumed that 16 32-bit words were allocated for every _possible_ TX endpoint, when only active endpoints should be considered.

See stm32-rs/synopsys-usb-otg#26

Co-authored-by: David Goodlad <david@goodlad.net>
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.

2 participants