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

stm32_fsdev & ISO EP buffer allocation improvements #1828

Merged
merged 32 commits into from
Feb 28, 2023

Conversation

HiFiPhile
Copy link
Collaborator

@HiFiPhile HiFiPhile commented Dec 29, 2022

Describe the PR
For stm32_fsdev:

  • Minor fixes
  • Rework FIFO based transfer
  • Add an endpoint allocator to overcome ISO ep limitation

For ISO transfer:

  • Add dcd_edpt_iso_alloc() / dcd_edpt_iso_activate() for ISO EP buffer allocation
  • Handle new allocation method in UAC2.

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

skuep and others added 19 commits December 29, 2022 12:13
…edpt_number in the stm32 device driver as a crude tool to control mapping of the endpoint address to actual endpoint register
… through lookup table. Allocation of endpoint is now only performed in dcd_edpt_open
* Correct handling of SOF interrupt
@HiFiPhile
Copy link
Collaborator Author

HiFiPhile commented Dec 29, 2022

I think it's better to add a flag somewhere to choose new buffer allocation method or continue use old one.
For example in tusb_mcu.h add TUP_DCD_ISO_ALLOCATION, or add it inside audio_device.c like how USE_LINEAR_BUFFER is handled. Some MCU don't have allocation problem so there is no need to touch them.

@HiFiPhile HiFiPhile force-pushed the stm32_fsdev branch 2 times, most recently from 4f59a1e to e833426 Compare December 29, 2022 20:22
@HiFiPhile
Copy link
Collaborator Author

@kkitayam Hi, I'm working on ISO EP buffer allocation based on #540, since video class is also ISO, does it benefit from this ?
Is this new dcd_edpt_iso_alloc() / dcd_edpt_iso_activate() suitable for video ?

@kkitayam
Copy link
Collaborator

@HiFiPhile

Hi, thanks for your comments.

Maybe yes, the APIs would be suitable for UVC too.
My understanding is that UVC controls ISO EPs in basically the same manner as UAC.

By the way, UVC can also use bulk EPs instead of ISO EPs for non-linear editing. Current UVC implementation has not supported this.
If we try to add support bulk EPs for UVC, we might be need some APIs like as dcd_edpt_iso_alloc() / dcd_edpt_iso_activate() for bulk EPs. It's just a guess. I am not familiar for bulk video data.

@HiFiPhile HiFiPhile force-pushed the stm32_fsdev branch 2 times, most recently from 51dc934 to 2ef5adb Compare December 30, 2022 14:32
@PanRe
Copy link
Collaborator

PanRe commented Dec 30, 2022

Yes i think this is a good idea! For us it might be a bit more work but the user does not need to be aware of this new allocation feature and we thus will get less issues reported ;)

Great work btw!

@hathach
Copy link
Owner

hathach commented Dec 31, 2022

Thank you @HiFiPhile , am on travel, will check this out next week.

TU_ATTR_WEAK uint8_t tu_stm32_edpt_number_cb(uint8_t addr);

// This callback is called on SOF and can be used to e.g. capture a timer value for timing purposes
TU_ATTR_WEAK void tu_stm32_sof_cb(void);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@skuep

For the moment I kept your SOF callback but for the stack it's not quite the right place to do it.

Did you test the tud_audio_feedback_interval_isr and found out it has too much jitter ?

If a prioritized SOF callback is needed we need do it in a generalized way for other DCD drivers.

Copy link
Contributor

@skuep skuep Jan 2, 2023

Choose a reason for hiding this comment

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

Yeah I am also still on the fence with this callback.
From the looks of it, the generated feedback values in the speaker feedback endpoint had way less jitter for the UAC2 application. However I did not check if this actually makes a difference for the (stability of the) feedback control loop running on the Host. Since I can only test Linux hosts, I left it in there for a better gut feel in terms of compatibility.

I do however think it would be a nice addition, if this would be a more general solution across all DCD drivers. The way I use it is that the callback "emulates" a timer capture on SOF (since the STM32 I used cannot do this in hardware). I fear however, that a more generalized solution would come with more overhead, since It would require a "callback sharing" mechanism, in case multiple USB classes use this callback. Which of course diminishes the purpose of this callback.
EDIT: Maybe it would make sense to integrate a SOF-timestamp function into the device drivers, that the tinyusb stack can then query? Of course it would need to be configurable what Timer to use, etc... Which again depends on the actually device used and it's device driver.

If I get around to it, I can test if it would work without this callback. I think it would be okay for now to remove the callback. If I desperately need it, we can discuss the issue further.

@skuep
Copy link
Contributor

skuep commented Jan 5, 2023

Sorry for the delay in testing your branch. However I have found a serious bug in the code that you based your commits on.
I updated the original branch that you used. See the very last commit here:
master...skuep:tinyusb:0.14.0-sk

I first noticed sporadic corrupted memory on only some of my devices, which was really hard to debug. I narrowed it down to the TU_ASSERT that I added to the code in this commit. It turns out, that you need to set the receive buffer-size with ISOCHRONOUS endpoints for both double buffer count registers in the buffer descriptor table. I only set it for the USB_COUNTn_RX register, because the fields are not even there in the manual for the corresponding USB_COUNTn_TX register. Turns out, they are there after all and need to be set. Else wildly varying (unitialized?) number of received bytes where reported for every other packet, leading the dcd_read_packet_memorycall to write more buffer than it was allowed to.

@HiFiPhile
Copy link
Collaborator Author

@skuep Thanks for your update ! Indeed I have only tested IN EP.

skuep and others added 2 commits January 5, 2023 16:23
…ommit gets triggered for ISOCHRONOUS endpoints. It is necessary for those endpoints to set the NUM_BLOCK and BLSIZE for the receiving buffer in both, USB_COUNTn_TX and USB_COUNTn_RX. Despite the datasheet showing those fields only for the USB_COUNTn_RX register
@skuep
Copy link
Contributor

skuep commented Jan 8, 2023

Alrighty!
I have switched my tinyusb submodule reference to your branch. I adjusted the SOF timing measurement code (due to removal of the tu_stm32_sof_cb callback. What can I say, it works very well so far!
The PMA and hardware endpoint allocation works quite nice, verified everything looks good in the xfer_statusmap.

From my side of view, testing was successful! Thanks a lot

@HiFiPhile
Copy link
Collaborator Author

@skuep
That's a great news !

PS: One thing very useful is J-Scope, I used it to visualize FIFO level while adjusting feedback loop.

@HiFiPhile HiFiPhile marked this pull request as ready for review January 8, 2023 14:37
Copy link
Contributor

@skuep skuep left a comment

Choose a reason for hiding this comment

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

Tested successfully with the AIOC project
https://github.com/skuep/AIOC

Copy link
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!! Sorry for huge delay, I am in the middle for reviewing this PR. Everything looks great, I am doing mostly clean up to make it more readable. Number of #ifdef in audio driver is starting to look scary now. We should do some refactoring in the future. Overall this PR is awesome and ready to merge, however there is a note in the review, that I think we should change to prevent a read out-of-bound of an array.

src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c Show resolved Hide resolved
Copy link
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.

everything looks good. Thank you very much for putting your effort to this brilliant PR.

@hathach hathach merged commit 3c38c7d into hathach:master Feb 28, 2023
@skuep
Copy link
Contributor

skuep commented Feb 28, 2023

Awesome! Thanks 👍🙂

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.

5 participants