USB using Clock v2

[supersimple33]

Summary

I have migrated the UsbBus to Clocks V2. That said I don't have a lot of experience with this stuff and I wasn't sure what I should consume and what I should hold onto or if this is even the right way to go about this. That said I tested my changes on the gc4 and they worked.

See #912

[kyp44]

This looks really good in terms of how it uses the clock::v2 API. The only thing I would add is that, according to the documentation, the Pclk is required to be at 48 MHz, so I would recommend we check for this, which can only be done at runtime. I'm not sure whether it makes sense to panic if the rate is not 48 MHz, or to have usb::UsbBus::new return a Result instead. This should probably involve a UsbError enum similar to adc::BuilderError for example.

[rnd-ash]

@supersimple33 @kyp44 the USB Clock check for 48Mhz is something i opened in #884 . So maybe look at that PR for inspiration should it not get merged

[kyp44]

@supersimple33 I would recommended checking the clock rate and returning a Result as in PR #884, since it was decided there that that was desired when migrating to clock::v2. However I would rename the UsbBusErr enum maybe to UsbBusError, noting that it is also private in the PR and it needs to be public. Be sure to also grab the updated documentation for UsbBus::new mentioning the change.

[ianrrees]

I'd suggest moving the comment about clock recovery mode in to the docstring for the method, so that it shows up in docs (or Rust Analyzer etc). Also, I'm pretty sure it's required to use USBCRM if running USB with the DFLL clock source, so not sure "more accurate" is the right way to describe it.

[kyp44]

A number of the USB examples for BSPs or their examples are now broken. This can be checked locally by running the build-all.py script in the root. I'm not sure what the policy is, but I'm guessing this PR should update them all to use the new API wherever possible. Partial v2 implementation if other v1-only peripherals are also used in examples may make this difficult or impossible. Not sure how to handle these cases.

[ianrrees]

Yes, the examples need to be updated for the T1 BSPs, but I don't think we need to keep them all building for every commit in the history. IOW, if some examples need peripheral A and B both updated to clock v2, we could leave the examples broken in the peripheral A PR, base the peripheral B PR on to the A branch, and fix the examples there.

The USB examples are problematic in their use of static mut references as well; we should fix that independently of the clock v2 work. It would probably be a little easier to do that first, rebasing this on top of the static mut ref fix.

[kyp44]

@ianrrees How can you base a PR on another branch since the branch only exists in the forked repository? Asking because I could base my PR #925 on this branch instead so that I can fix the nvm_dsu example in the feather_m4 BSP in my PR.

[ianrrees]

@kyp44 I'd suggest that while working the Git stuff out, you keep one copy of your work based off master as usual, so you're a bit freer to experiment with other copies. To make the backup, with your branch checked out just do:

git branch backup-mybranch

You'll then need to make your local repo aware of the repo that has the work you want to rebase yours on to. In this case, I guess that'll be supersimple33's GH - add that as a remote like:

git remote add supersimple33 [email protected]:supersimple33/atsamd.git
git fetch supersimple33

Finally, you can rebase your work on to a commit from that branch. Keep in mind that in Git, a branch is just a named pointer to a commit (if you haven't come across Git Internals, I feel like it's well worth the time to read it closely and work through the examples).

git rebase supersimple33/UsbV2

[kyp44]

@ianrrees But then my PR will include changes from the other PR as well. Is that not a problem? Does it mean that the other PR has to be merged first?

[ianrrees]

@kyp44 You're right that if one PRed branch (for feature A) is based on another PRed branch (for feature B) that the first PR will look a lot bigger (the diff between master and A+B). I don't think that's inherently a problem, but it does require a bit more Git-fu to work through. I guess there are a few related things going on here:

If you're wanting a context that has both sets of changes, e.g. to update examples, then you could use this approach to update the examples but PR just your set of changes. This would involve maintaining two branches for A and A+B. A would be PRed, and the examples in it would not be expected to build (until B has been merged in to master and A is rebased on to it). In A+B you could get the examples to build, then use eg cherry-pick to bring that work in to A.

There are multiple approaches we can consider to manage the PRs that will be involved in the clock v2 migration. One option is to proceed as usual, with each PR being a self-contained change to master and updating the T1 BSP examples. Another is to have a clockv2 branch that we PR changes to for each peripheral or whatever, and maybe in that we don't worry about keeping the examples in good shape as we go. Then, once that work is done, we just update master to that. Personally, I think that there's maybe a little more overall work in going with the first approach, but it's more robust and a safer option given our intermittent maintenance resources. Longer-lived feature branches (like clockv2 might be) can take on a life of their own, as work in master will continue. If we had a large amount of code that really couldn't be updated piecemeal, that would push toward a clockv2 branch, but I don't think the examples are really at that level so I'm inclined to stick with basically the normal approach of updating one peripheral at a time in master.

Third is the question of updating peripherals to clockv2 when they also need to keep working with eg thumbv6. I think the answer here is as you've suggested: support clockv2 on thumbv6 before working on the peripherals that need to work on both architectures.

[kyp44]

Gotcha, I think for the examples going forward (including the one discussed above for the DSU/USB migrations) I'll just make a note in the breaking PR and make a note to fix it once both have been merged. I think it would have been good to have done a clockv2 branch if it was done from the beginning as this would have avoided the current problematic state of partial support, and the full migration of the entire HAL could have been merged into master at once. At this point though, a number of peripheral migrations have already been merged to master, so probably not worth trying to do that now. Hindsight is 20/20.

I agree that it's it's worth waiting for the thumbv6 v2 port before tackling the affected peripherals instead of the half-migration done by the Adc. I'll note this in the tracking issue and mark the applicable peripherals. In the meantime, I'm going to continue working on the thumbv7 only peripherals, noting example breakages as I go, which I'll also note in the tracking issue.