Handle USB CLEAR_FEATURE

[kev009]

The current firmware hangs after the first gw run, because umodem(9) sends a CLEAR_FEATURE. I think this is an appropriate fix.

[keirf]

Thank you for tracking this down! Actually it looks like I must support set and clear of Halt state for all endpoints, strictly speaking. So I may as well do that. I will report back with the result of implementing that, so you can test it.

[kev009]

@keirf good call, this was only working every other command as is. The reset requires carefully setting up the double buffering, I don't think there's an easy way to do it, but here is an attempt for you to consider.

Also and seperately handle DTR.

[keirf]

That's a lot of code, though it definitely looks plausible, how tested is it or is it imported from another project?

[kev009]

It's based on the existing usbd_configure_ep (

greaseweazle-firmware/src/usb/hw_usbd.c

Line 194 in 75bd97a

static void usbd_configure_ep(uint8_t ep, uint8_t type, uint32_t size)

and

greaseweazle-firmware/src/usb/hw_usbd_at32f4.c

Line 219 in 75bd97a

static void usbd_configure_ep(uint8_t epnr, uint8_t type, uint32_t size)

).

It definitely needs to be tested on the existing platforms (Linux, Windows, Mac?) because FreeBSD was awkward here. I also only have the at32f4 target but the others are compile tested.

[keirf]

Okay gotcha, I think actually this should be clear_stall as a companion to stall, and that even the stall side probably needs some work. I can crib from configure_ep as you did, plus TinyUSB which does a more comprehensive job than I currently do. I will update here.

Regarding DTR support -- was that necessary or a nice to have? It kind of feels sensible, but the sort of change that can create unexpected problems in specific configurations...

[kev009]

The DTR helps when the commands are severed, where ClearComms in the client didn't get a chance to run. Probably never seen before, I think it is correct, but not strictly necessary with the other change since it is unlikely in the normal flow outside of this platform triggering the breakage.

[keirf]

Could you please test branch named handle_stall: https://github.com/keirf/greaseweazle-firmware/tree/handle_stall

[kev009]

@keirf it gets stuck

$ gw update --force --file /usr/home/kev009/c/greaseweazle-firmware/out/at32f4/prod/greaseweazle/target.upd
Updating Main Firmware to version 1.6...
Done.
load: 1.10  cmd: python3.11 35736 [select] 17.62r 0.24u 0.02s 0% 35400k
load: 1.10  cmd: python3.11 35736 [select] 18.51r 0.24u 0.02s 0% 35400k
load: 1.14  cmd: python3.11 35736 [select] 38.86r 0.24u 0.02s 0% 35400k

Unlike the master branch it never responds to the first command after replug, no commands get through. I need some rest before I can dig back in.

[keirf]

No worries I've clearly just broken your code by ripping chunks out of it ;) It's obviously a pain that I don't have a testbed that issues these CLEAR_FEATURE requests. Possibly I should add some stress testing of it to greaseweazle-testbed, which is the factory test for Greaseweazle: I could add more protocol tests in a "dev test" version of that project.

Then I could see that out-of-the-box Greaseweazle doesn't work, then see that it works with your patch, and furthermore test that I don't regress things when I muck with the code. It would also let me test your patch on other Greaseweazle boards where let's be honest it's quite likely there are still problems as this stuff is almost impossible to get right outside of a hack-compile-test loop.

By the way what wIndex values (ie. endpoint numbers) do you see being passed to CLEAR_FEATURE(ENDPOINT_HALT)?

[keirf]

PS I am also happy to send you a tester board for Greaseweazle. I have a small box of them, and they are of little use to most people!

[keirf]

Oh and another obvious option for me I suppose would be to try out FreeBSD in a VM with USB passthrough

[kev009]

Yeah a passthru VM should work. Note that our py-serial is currently broken so I have a temporary workaround in the py-greaseweazle: https://github.com/freebsd/freebsd-ports/blob/main/sysutils/py-greaseweazle/files/patch-src-greaseweazle-tools-util.py#L6. You will need something similar if you just check out the python code and build it yourself. This will be fixed soonish so I didn't bother with a PR, local ports patch to workaround until then.

I added one followup commit on top of yours to this PR. I think the first 3 items are critical, the IRQ protection might not be necessary here but matches the donor code. All the other files changes look fine to me.

[keirf]

Do you think the strategy of aligning clear_stall with configure_ep is a generally good one? Perhaps that could mean we share code, and could be a strategy applied to all three usb-peripheral implementations.... I think perhaps only buffer allocations would need to be skipped?

Which endpoints are you seeing having the clear feature request applied to them?

[kev009]

I don't think there's a great opportunity. Might be able to save a tiny bit of code on the dbl_buf branches but the reset is subtly different than init overall. If you see something more minimal I can test again.

wIndex 0x0002 (host to device, bulk data) and 0x0083 (device to host, bulk data)

[kev009]

Any update? I was imaging it would be stabilized quickly but now I think I prematurely committed the software to ports.

[keirf]

I must admit I haven't got round to it, sorry. Perhaps you could commit a patched firmware to your package?

[kev009]

I could try that but it would probably require the build toolchain instead of fetching a random binary. Is there anything I can do to make the review or testing easier?

[keirf]

It's not so much review as getting more test on these changes. The firmware changes very little these days, especially in these low-level and (to me) less understood peripheral-specific parts.

Do you have a plan for how to guide users to update their firmware to one with the fix? I assume that an unpatched Greaseweazle can't even be gw update on FreeBSD? Would you be suggesting a DFU update, or update on some other platform first?

Also are there restrictions on the sources of binaries that you can recommend? Is the official Greaseweazle-firmware release zip okay, but other sources not? Perhaps I could put the fixes on a branch and build them as an alt firmware included in the release zip? Or make a secondary release zip?