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

General USB fixes ported from the new mmu bootloader #323

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leptun
Copy link
Collaborator

@leptun leptun commented May 4, 2024

Saves quite a bit of flash and the usb still works fully.

@leptun leptun requested a review from gudnimg May 4, 2024 18:26
Copy link

github-actions bot commented May 4, 2024

All values in bytes. Δ Delta to base

ΔFlash ΔSRAM Used Flash Used SRAM Free Flash Free SRAM
-114 0 28256 1669 416 891

Copy link
Collaborator

@gudnimg gudnimg left a comment

Choose a reason for hiding this comment

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

Works well on my end. I flashed multiple times and also downgraded/upgraded multiple times. No issues.

/** Endpoint address of the CDC device-to-host data IN endpoint. */
#define CDC_TX_EPADDR (ENDPOINT_DIR_IN | 3)
#define CDC_TX_EPADDR (ENDPOINT_DIR_IN | 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 just a thought - can't we run into mysterious issues with flashing the MMU board on some platforms? Hopefully not, USB arbitration should be hidden from the avr-dude, but who knows?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the endpoint structure is hidden from avrdude. It's the OS CDC class driver that handles this. As for whether it can be a problem, I very much doubt it. There is no hardcoded order in which endpoints have to be allocated, so any driver should handle this. The only maybe risky driver is the custom one on Windows that applies the MMU product name instead of a generic name to the virtual com port, but that seems to be working just fine on my end, so I doubt there will be any issue caused by this.

@@ -11,7 +11,7 @@
#define NO_INTERNAL_SERIAL
#define NO_DEVICE_SELF_POWER
#define NO_DEVICE_REMOTE_WAKEUP
// #define NO_SOF_EVENTS
#define NO_SOF_EVENTS
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 this sounds dangerous, we had tons of trouble with SOF events on Buddy platform...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SOF is not used by the CDC class. In device mode, only stuff that does isochronous streams of data cares about SOF, such as UAC and UVC. So disabling the handling of the flag in the interrupt has no adverse effect. We don't care about the SOF callback.
On the other hand, SOF is generated by the host for any class and is part of the frame structure of USB. Maybe this is what you are remembering?

Copy link
Collaborator

@DRracer DRracer left a comment

Choose a reason for hiding this comment

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

Generally, saving 114 bytes just by stripping some (potentially) unneeded features from the USB lib sounds nice, but I have no idea what may break, esp. considering various platforms we are connecting to. Remember, people are reporting "cannot flash the FW" occasionally a we haven't been able to reproduce these issues at all.

I'm not against merging it, it just needs tons of testing.

@leptun
Copy link
Collaborator Author

leptun commented May 6, 2024

SOF events are not used by the CDC class. The reordered endpoints should be fine since the descriptor was updated. In any case, this change is just so that the mmu firmware config matches the one in the bootloader.

@leptun
Copy link
Collaborator Author

leptun commented May 12, 2024

Remember, people are reporting "cannot flash the FW" occasionally a we haven't been able to reproduce these issues at all.

This seems to be caused by the transition from the MMU fw to the bootloader. So when the usb device disconnects and then reconnects. This PR doesn't touch that in any way. I have yet to identify what exactly causes that problem since I can't reproduce the problem consistently. Any attempt to cause it in rapid succession makes the problem go away completely.

I'm not against merging it, it just needs tons of testing.

What do you propose for testing this? I've tested updating the firmware on windows and that worked correctly. I can also run tests on macOS on my thinkpad and on linux. Other than that I don't know what else to test.

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