zephyr
zephyr copied to clipboard
zephyrproject-rtos
usb: device_next: Simple NCM driver for usb-next
This is a first version of an NCM driver for usb-next. As a template the usb-next ECM driver has been used.
The driver has been kept very simple (one datagram per NTB and one buffer per direction). Current idea is not to optimize performance, focus is more on existence of an NCM driver for Zephyr.
To make the driver work with iperf (my testcase), CONFIG_NET_BUF_VARIABLE_DATA_SIZE=y must be used.
Discussion about this PR can be found in https://github.com/zephyrproject-rtos/zephyr/issues/71451.
Resolves: #71451
I am nowhere close to a maintainer, and this is an unsolicited review, but I am trying to anticipate some points in hope to save you some time.
Many thanks for reviewing the code!
* give the coding style a look https://docs.zephyrproject.org/latest/contribute/guidelines.html#coding-style eventually applying `clang-format -i` to all C source/headers to save you time manually reformatting everything, and the result will be matching the expectations.
Currently working on this. Iterating thru the code until the runner no longer complains (which seems to be a painful process)
* submit several pull-requests, for instance one with just the header definitions from the standard
Unfortunately all those changes depend on each other. So there is no meaning of providing e.g. just the headers.
I realize that a lot of my comments might as well be addressed to USB CDC ECM, and open questions for the USB stack in general. In hope you can get more folks offering some review soon. I can ping the #pr-help channel to draw attention on a pull request that needs some review when you wish.
I do like how this class is a thin wrapper between the Ethernet and USB Class APIs.
I realize that a lot of my comments might as well be addressed to USB CDC ECM, and open questions for the USB stack in general. In hope you can get more folks offering some review soon. I can ping the
#pr-helpchannel to draw attention on a pull request that needs some review when you wish.
Yes please, that would be very kind.
I do like how this class is a thin wrapper between the Ethernet and USB Class APIs.
The ECM driver is even more direct (BTW Johann Fischer is the author of that). NCM has the capability to additionally packing multiple datagrams into one transfer block. But as stated in the intro this is currently not implemented.
Actually for me the nice thing is, that there are NCM drivers even for Win10.
You might be asked to merge several commits together. If that were to happen, do feel free to reach me at [email protected] (or josuah.demangeon on the chat) so that we can get this as expected by the real reviewers.
Please fix the commit subjects, now they are all the same which is not good. The commit subject should indicate what the commit is changing.
Please fix the commit subjects, now they are all the same which is not good. The commit subject should indicate what the commit is changing.
Done that and squashed everything. Top be honest, commit message policy is not clear to me. If one does not use the special subject structure, then one of the CI checks complains, also it seems that only squashed PRs are merged, so the history of a PR seems to be irrelevant.
it seems that only squashed PRs are merged, so the history of a PR seems to be irrelevant.
This is definitely not true, the commit history is very much relevant. The commit messages rules are actually very simple, the individual commits should contain logical changes related to each other. For new code, in many cases there is need to have only couple of commits, for example the actual feature in one, tests in another, samples in another. Sometimes it makes sense to split the feature to multiple commits each implementing sub-features. It really depends what is being submitted.
it seems that only squashed PRs are merged, so the history of a PR seems to be irrelevant.
This is definitely not true, the commit history is very much relevant. The commit messages rules are actually very simple, the individual commits should contain logical changes related to each other. For new code, in many cases there is need to have only couple of commits, for example the actual feature in one, tests in another, samples in another. Sometimes it makes sense to split the feature to multiple commits each implementing sub-features. It really depends what is being submitted.
Thanks for the clarification, did not want to offend anybody.
I have to admit that I'm new to Zephyrs PR policy and thus have to get used to it.
https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-guidelines
To help with maintenance and troubleshooting, I believe Zephyr maintainers like to have only meaningful commits. Maybe making a single commit that introduces the class would work?
git rebase -i HEAD^4 and modifying the lines for the two commits that needs to be changed from pick to squash would likely work.
This is a recent modification on upstream Zephyr:
usbd_contexhad a typo and was adjusted tousbd_context.
Haha... first time I couldn't compile before pushing, because my environment was broken (see https://github.com/eclipse-cdt/cdt/pull/846). And then the CI shows you that that's a bad idea.
Squashed the commits (again). I actually don't like that, because history is lost (and actually I'm not a fan of rebasing because history is rewritten, but that is another story...))
@rgrr There still seems to be no changes since the last review, do you have any challenges with git?
@rgrr There still seems to be no changes since the last review, do you have any challenges with git?
No... sorry... I'm currently too busy to work on the PR. Don't drop it, if there is time I will polish it.
Following this initial effort on bringing CDC NCM to Zephyr, an independent PR was started after this message and is now merged:
- https://github.com/zephyrproject-rtos/zephyr/pull/79508
The outline of functions is the same, but their implementation differ in several places. I suppose this PR content helped in giving insight in how the problem could be solved, and was supportive on the other PR which led to CDC NCM merged Zephyr.
Thank you @rgrr ! It might be possible to close this PR now, as the NCM development effort were pursued on the other PR.
:
The outline of functions is the same, but their implementation differ in several places. I suppose this PR content helped in giving insight in how the problem could be solved, and was supportive on the other PR which led to CDC NCM merged Zephyr.
Great, if my implementation was of any help!
Thank you @rgrr ! It might be possible to close this PR now, as the NCM development effort were pursued on the other PR.
Of course close it. I will check the other implementation soon and provide comments if required.
Sorry for wasting your time for reviewing my PR.
Sorry for wasting your time for reviewing my PR
No issue at all ! I learned in the process and that was useful in other ways (UVC).