-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
usb: device_next: Simple NCM driver for usb-next #72310
base: main
Are you sure you want to change the base?
Conversation
a4b0e57
to
94b376e
Compare
You might be asked to merge several commits together. If that were to happen, do feel free to reach me at me@josuah.net (or |
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. |
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. |
uint8_t ep; | ||
int ret; | ||
|
||
LOG_DBG(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this would receive similar comment to this:
#53408 (comment)
A git commit --amend
adding some short description of just a few words would allow keeping these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed every empty LOG_DBG(), wondering if anybody ever made the cost/environmental calculation of these CI runs caused by mini changes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read somewhere that scripts/rules were setup to avoid extra CI cycles, with manual or auto-run CI.
Maybe there can be rules to only auto-run CI when the PR is not in "draft" state.
33e603f
to
387704e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a recent modification on upstream Zephyr: usbd_contex
had a typo and was adjusted to usbd_context
.
To help with maintenance and troubleshooting, I believe Zephyr maintainers like to have only meaningful commits.
|
Haha... first time I couldn't compile before pushing, because my environment was broken (see eclipse-cdt/cdt#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...)) |
@@ -41,6 +41,8 @@ instance (`n`) and is used as an argument to the :c:func:`usbd_register_class`. | |||
+-----------------------------------+-------------------------+-------------------------+ | |||
| USB CDC ECM class | Ethernet device | :samp:`cdc_ecm_{n}` | | |||
+-----------------------------------+-------------------------+-------------------------+ | |||
| USB CDC NCM class | Ethernet device | :samp:`cdc_ncm_{n}` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move changes in doc/connectivity/usb/device_next/usb_device.rst to a separate commit, it would also make it easier for you to rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok... I have created three branches and soon two more PRs will appear here. New code stays in this PR.
Concerning review procedure: who is resolving conversations? You? Me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One branch (one PR) with N commits is the preferred way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gosh... what have I done!? Misunderstood Johanns point completely. Will go back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what commit comments do you expect in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If by commit comments you mean the commit messages, then each commit message should describe what the individual commit does. Do not reuse the title for all of the commits, make each of them point to the respective part. For example the commit changing sample should start its commit title with samples: net: zperf:
.
Things like "This PR contains the actual driver." do not belong in commit message.
@@ -199,7 +202,7 @@ struct cdc_acm_notification { | |||
} __packed; | |||
|
|||
/** Ethernet Networking Functional Descriptor */ | |||
struct cdc_ecm_descriptor { | |||
struct cdc_eth_functional_descriptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think you should rename it here or at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... but cdc_ecm_descriptor
is the wrong name. Will change it back anyway
CONFIG_NET_PKT_RX_COUNT=14 | ||
CONFIG_NET_PKT_TX_COUNT=14 | ||
CONFIG_NET_BUF_RX_COUNT=28 | ||
CONFIG_NET_BUF_TX_COUNT=28 | ||
CONFIG_NET_BUF_DATA_SIZE=1500 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration overlay is not used if sample is build for the new USB device support. Stray changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to rethink why I have done this ;-)
CONFIG_USB_DEVICE_STACK_NEXT=y | ||
|
||
# next-ncm does not work well with the static-size buffers | ||
CONFIG_NET_PKT_BUF_RX_DATA_POOL_SIZE=10000 | ||
CONFIG_NET_PKT_BUF_TX_DATA_POOL_SIZE=10000 | ||
CONFIG_NET_BUF_VARIABLE_DATA_SIZE=y | ||
|
||
CONFIG_LOG=y | ||
CONFIG_USBD_LOG_LEVEL_WRN=y | ||
CONFIG_UDC_DRIVER_LOG_LEVEL_WRN=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move sample changes to a separate commit.
Looks like there should be three commits in your PR, 1) USB NCM implementation + bindings, 2) sample changes, 3) doc update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done that
/ { | ||
cdc_ncm_eth0: cdc_ncm_eth0 { | ||
compatible = "zephyr,cdc-ncm-ethernet"; | ||
remote-mac-address = "00005E005301"; | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indent with tabs, always.
/* | ||
* check (first) NDP(16) | ||
*/ | ||
const struct ndp16_t *ndp16 = (const struct ndp16_t *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable definition should be at the top of a block. Please clean up and fix your code, I will continue the review after that.
|
||
|
||
|
||
static bool _cdc_ncm_frame_ok(struct cdc_ncm_eth_data *data, struct net_buf *const buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefixing with _ is not okay.
#define CFG_CDC_NCM_RCV_NTB_MAX_SIZE 3200 | ||
|
||
/* Table 6.2 Class-Specific Request Codes for Network Control Model subclass */ | ||
typedef enum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No typedefs.
#ifndef CFG_CDC_NCM_ALIGNMENT | ||
#define CFG_CDC_NCM_ALIGNMENT 4 | ||
#endif | ||
#if (CFG_CDC_NCM_ALIGNMENT != 4) | ||
/* headers and start of datagrams in structs have to be aligned differently */ | ||
#error "CFG_CDC_NCM_ALIGNMENT must be 4" | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not the same as just #define CFG_CDC_NCM_ALIGNMENT 4
?
Please remove all the CFG_
prefixes and move whatever make sense to Kconfig options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have "reserved" those CFGs for future extensions. Currently there is no meaning in them, so they don't appear in Kconfig. Will change it.
|
||
#define NTH16_SIGNATURE 0x484D434E | ||
#define NDP16_SIGNATURE_NCM0 0x304D434E | ||
#define NDP16_SIGNATURE_NCM1 0x314D434E |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define NTH16_SIGNATURE 0x484D434EUL
#define NDP16_SIGNATURE_NCM0 0x304D434EUL
#define NDP16_SIGNATURE_NCM1 0x314D434EUL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfischer-no : many thanks for the extensive review, I will change accordingly (or comment on it). But this will take some time.
This is a simple USB-NCM driver. It just has one NTB per direction and within the NTB only one datagram can be received/transmitted. Main goal is existance of an NCM driver, performance will follow in further steps. This PR contains the actual driver. Signed-off-by: Hardy Griech <ntbox@gmx.net>
Sample changes. Signed-off-by: Hardy Griech <ntbox@gmx.net>
Documentation changes. Signed-off-by: Hardy Griech <ntbox@gmx.net>
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 #71451.
Resolves: #71451