shared/tinyusb: Add common cdc tx/rx functions. #14462
Conversation
|
Code size report: |
b4de5d4 to
d606676
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14462 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 161 161
Lines 21204 21204
=======================================
Hits 20864 20864
Misses 340 340 ☔ View full report in Codecov by Sentry. |
d606676 to
278ef2d
Compare
There was a problem hiding this comment.
This is really useful, thanks @andrewleech! One small change suggestion, but I think we should merge it with or without the additional name change.
4f590cb to
754ae8b
Compare
|
Note: I've only been able to test this on RP2, I don't own samd / mimxrt / renesas-ra boards. edit: I've got my hands on samd, mimxrt and renesas-ra boards ( thanks @mattytrentini ) but haven't been able to figure out how to enable usb on renesas yet... renesas#2 So now rp2, samd21, samd51, mimxrt have been tested and appear to work as expected! |
52361d3 to
5bedc84
Compare
It may be redundant as I saw the follow-on PR has been tested on samd already, but I've tested this PR on samd and it seems to work as expected. 👍 |
Thanks I've just updated my previous post to reflect what I've now been able to test! Regardless, I certainly appreciate any additional testing as I'm not very familiar with some of these ports! FYI thanks to this incredibly helpful tidbit of info, I'm also looking at bringing esp32s2 and s3 into using this same shared tinyusb implementation, as well as fixing the auto-bootloader jump there for S3 which is currently incredibly frustrating! It's probably worth saving the esp stuff for a follow up PR though, it's a larger parcel of work though! |
|
@andrewleech Oh wow, that's fantastic news! I had anticipated this being a much bigger task, very exciting. |
|
So now I've tested everything here except the renesas port changes. Can anyone provide guidance on how to update the auto generated files in a renesas board (in particular the interrupt vector header) to enable USB on a evk board? @TakeoTakahashi2020 @dpgeorge Alternatively @iabdalkader could you perhaps test this PR doesn't break anything on a Portenta C33 (I can really afford one myself for testing with). |
5bedc84 to
41a54fa
Compare
|
@andrewleech Yes I can test it, perhaps later today. What do I need to test ? |
Thanks, on this PR there's no intended functional change so it's just a basic check that stdio / repl still seems to work. If possible, the bigger change is in the follow on PR #14485 If you know, I'd love some pointers on how to create (or better, modify) the auto generated code in a renesas board so I can test / document it myself too. |
REPL is still functional on the Portenta C33.
You'll need to install the e2studio, load the configuration, edit and then generate the code. For this you need the configuration.xml, I think it's this one: configuration.xml.zip |
Ok thanks! I previously installed RA Smart Configurator which can open a "project", not sure if that will be the XML. I'll try tomorrow. I really think this XML/project file needs to be included in the board folder as the source of the auto gen files. |
That's great thanks! |
There's a cfg.txt file too, those are the only two non-generated files. |
|
Thanks @iabdalkader that "configuration.xml" file opened just fine in the standalone "RA Smart Configurator" application! That got it going for me, with a cut up usb cable wired to the USB pins and a jumper from 3v to vbus to enable it I had cdc working (quicker than wiring resister divider from USB 5v to vbus pin). More than just replicating your testing on this PR, this more importantly helped me finish off the updates in tinyusb to support my startup buffer feature. Thanks again! I'll try to get a more succinct version of these notes into a readme update pr to help the next person. Would you mind me also including your configuration.xml in that pr (in the matching board folder) to reference as an example? |
No please go ahead. The configuration should have been there with the board files. |
41a54fa to
2313625
Compare
|
I found a further consolidation that made sense in moving more of the cdc stdin handling into the With that change, I've restested on all platforms covered here and all appear to be working (basic read/write on repl and mpremote mount still works). I previously had nrf also included here but it was a larger change which grew even larger as I tested and cleaned it up so moved it to its own PR. With this, I consider this PR complete and ready for final review. |
There are a few TinyUSB CDC functions used for stdio that are currently replicated across a number of ports. Not surprisingly in a couple of cases these have started to diverge slightly, with additional features added to one of them. This commit consolidates a couple of key shared functions used directly by TinyUSB based ports, and makes those functions available to all. Signed-off-by: Andrew Leech <andrew@alelec.net>
Signed-off-by: Andrew Leech <andrew@alelec.net>
Signed-off-by: Andrew Leech <andrew@alelec.net>
Signed-off-by: Andrew Leech <andrew@alelec.net>
Signed-off-by: Andrew Leech <andrew@alelec.net>
e7b4332 to
c11efc7
Compare
|
Thanks, this looks really good. It was always on my list to eventually factor these functions, and I'm glad someone else got to it first :) |
|
Using pull from 31 may 2024, and master branch, NRF52840 does not compile with these changes on my system. It cannot find the and My code now compiles (BOARD=SEEED_XIAO_NRF52), and the REPL works with new build. Note that the 1.23.0 tag compiles and runs without modification. I do not think these are the optimal patches as I did them simply to resolve link/compile errors based on the samd code... and I have little knowledge of |
|
Not surprisingly, the @andrewleech |
|
Sorry for the breakage @ricksorensen and @robert-hh ! If either of you get a chance could you take a look at #15158 ? In that follow up PR I remove the nrf specific USB CDC file and consolidate all this functionality. But yes it makes sense to get the master build fixed quickly with or without that PR |
Will do. Rebasing to the current master branch fails as well due to conflicts in the make files of the other ports. Nothings difficult, just duplicating references to files, but it breaks rebase. |
|
For me both boards compile okay with #15158. (Time on history is CDT (-5) I built yesterday but did not have access to my SEEED_XIAO_NRF52 board until this morning ... the REPL works. The only difference I see in the nrf port is in the I also tried the pull request from this morning (#15189) which also worked. Same differences as above. |
|
The esp32 and nrf build issues introduced by this PR should be fixed by #15189. |
The Portenta C33 firmware has been broken after 1.23.0. It's likely that these changes and following ones, broke something. Also in 1.23.0 WiFi hangs. The last known good release was 1.22.1 |
Can you give more details about the failures? Does repl not respond? Or is it related more to the 1200bps bootloader? Wifi is a tricky one, maybe at least one of the boards using an esp-coprocessor based wifi could be added to the boards @dpgeorge or others involved in release testing has for running multitests on? Though I'm aware that's a laborious process already... hopefully the automated testing discussed in the meetup the other night can progress to help here! oh, is the failure you're referring to the compile error? Maybe this is a board that should be in CI, it's arguably the renesas board with the most features currently so covers the widest range of supported libraries for that port really? |
No, the firmware builds fine but USB doesn't enumerate. When the firmware is loaded and the board exits DFU the device becomes dead.
I think it's already in CI. |
You're right, it is. It was a submodules issue on my end with the compile. USB not enumerating at all? That sounds like either
I just retested master-ish on the " |
|
@iabdalkader if you can it would be helpful to test #15550 to see if that changes anything for you? Also you could try downgrading tinyusb submodule back to 1fdf2907 I think that's the commit it was for the previous release. |
|
I did but it's still not working. I also reverted d144f06 (had to comment out SOF enable), and doesn't work either. |
|
Reverting this 2d33071 FYI there's no issue with WiFi, it was just an old rev board with a broken crystal that made NTP/RTC get stuck. |
|
@iabdalkader that's really surprising that the shared code broke the USB support. There must be some difference between the FS and HS peripheral that's conflicting with that commit. If there's some way I could get my hands on a portenta I'd be happy to investigate the problem, though I don't know anyone local to me in Melbourne with one I could borrow. |
|
@andrewleech Somehow it has something to do with the RTC/LSE issue on the old revision boards. Before switching to the shared TinyUSB code, this board used to work. If things are working fine on production boards then it's not a problem. I've asked someone to test on a different board to confirm. |
There are a few tinyusb CDC functions used for stdio that are currently replicated across a number of ports.
Not surprisingly in a couple of cases these have started to diverge slightly, with additional features added to one of them.
This PR consolidates a couple of key shared functions used directly by tinyusb based ports.