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

usbdfu small tidy up #230

Closed
wants to merge 2 commits into from
Closed

Conversation

tormodvolden
Copy link

No description provided.

The only difference between these different implementations
is the pin configuration for pulling up the USB D+ wire,
and related clock setup.

Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
@karlp
Copy link
Member

karlp commented Nov 20, 2021

this is exactly why I'm not doing any changes like this, it's too easy to just have copypasta all over the place, I'm not touching these further in the same path.

@karlp karlp closed this Nov 20, 2021
@tormodvolden
Copy link
Author

Erm, what is your suggestion here? Can't you e.g. just pick the second commit, which improves documentation?

@tormodvolden
Copy link
Author

I find it a bit confusing that the 4 examples have these small differences. Wouldn't it make sense that for each difference there is one way that is better than the other, and the best is used in all examples?

A normal reflex here would be do make a common code, and just select board settings somehow. But I see the advantage of having everything compact in one file, especially for a educational example. What path do you see forward for these examples?

@karlp
Copy link
Member

karlp commented Nov 22, 2021

yes, you've understood the problems :)

common code and a board selector or set of boards is good for demonstrating a peripheral, but quite bad for being an easy to use self contained example. The current repo tried to do both, and has become largely unmaintainable copy pasta.

For instance, you've tried to synchronize things, but have actually missed stm32/f1/stm32-h103/usb_iap and stm32/f1/other/usb_hid which also use DFU.

The forward path is unclear, but a likely path is a split of two repos. One repo with "peripheral api" examples, with no duplicated code and a limited set of example platforms, much like the current libopencm3/tests/gadget-zero uses shared code for the usb stack. This repo largely doesn't exist. The other repo(s) would be like libopencm3-miniblink, where all/any boards are supported, so you can get an evaluation going quickly, but only a limited set of peripheral apis might be supported, that are available on all boards. This also has the "downside" of not being these single totally contained examples that the current repo has, but it's debateable whether that's actually been a worthwhile goal, given taht no-one should really be building their apps like that anyway.

@tormodvolden
Copy link
Author

I didn't miss stm32/f1/stm32-h103/usb_iap though :) The two HID examples are a bit different, but true, they could also need the same documentation update.

I guess the original problem that I want to help solving is, if you want to improve a duplicated example, where do you start? If they have been synchronized you can then apply the same change all over.

For instance if someone wants to add DFU to their project, it would be nice if libopencm3 would show the best example. But I do understand it is out of your scope to provide reference implementations of everything.

BTW, I now found #205, which seems to nicely add UPLOAD support. What should be done to get this in?

@tormodvolden
Copy link
Author

Someone said in #119: "The general problem with something like this is that it results in the examples being different. They really need to be more common, not diverging with this cdcacm example being completely different from the others."

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.

None yet

2 participants