-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Add new Network MIDI 2.0 host stack #93933
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
Conversation
jukkar
left a comment
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.
Looks great, I have mostly minor style related nits
|
Thanks @jukkar for your initial review 🙂 I just rebased on top of main, and pushed the following changes:
|
09578d5 to
d73056d
Compare
| } | ||
|
|
||
| /* 1. Start hashing with the session nonce */ | ||
| hash.in_buf = (uint8_t *) sess->nonce; |
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.
As a side note, this is currently const-incorrect.
This can be fixed if we merge #94218
|
|
||
| if (sess->ep->auth_type == NETMIDI2_AUTH_SHARED_SECRET) { | ||
| /* 2. Finalize hashing with the shared secret */ | ||
| hash.in_buf = (uint8_t *) sess->ep->shared_auth_secret; |
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.
same here
| net_buf_pull(buf, payload_len - NETMIDI2_DIGEST_SIZE); | ||
|
|
||
| /* 2. Continue hashing with the username */ | ||
| hash.in_buf = (uint8_t *) user->name; |
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.
Same here
| } | ||
|
|
||
| /* 3. Finalize hashing with the password */ | ||
| hash.in_buf = (uint8_t *) user->password; |
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.
And again same here
| @@ -0,0 +1,18 @@ | |||
| CONFIG_NETWORKING=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.
you probably want to enable CONFIG_TEST_RANDOM_GENERATOR somewhere here
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.
Oh I just see your comment now, while I just pushed a different solution.
Since only the mps2/an385 platform is causing problem, I excluded it from the tests definitions in the sample; but I don't believe this is the clean solution, only the one where the CI would pass.
Maybe we could enable CONFIG_TEST_RANDOM_GENERATOR instead, but only on boards that require it ? This looks like an unsafe option to enable unconditionally ?
Thank you for looking into that 👍
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.
yeah not entirely sure what story is but CONFIG_TEST_RANDOM_GENERATOR seems to be what's done in all other samples under net/. With the exclude it passes CI but then it'd break if someone tries to build the sample against a specific platform with no rng -- probably not a real problem, doubt there's anything out there with ethernet but no rng but I think there's value in keeping the net sample structure uniform.
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.
Yes indeed, keeping samples consistent is a very good reason to do so, so I just pushed that instead.
Thanks !
d73056d to
43270b2
Compare
|
...also, you could probably send the shield commit in its own PR if you want to get that one off the way of this one, it'd probably get in pretty quickly, up to you. |
43270b2 to
3fcca35
Compare
jukkar
left a comment
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.
Couple of minor nits. Also please do not resolve comments yourself but let reviewer do that. It helps the review process a bit as reviewer can mark the comment resolved when done.
See https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers for more info.
include/zephyr/net/midi2.h
Outdated
| /** Sequence number of the next universal MIDI packet to receive */ | ||
| uint16_t rx_ump_seq; | ||
| /** Remote address of the peer */ | ||
| struct sockaddr addr; |
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 works in zephyr because we allocate enough storage in struct sockaddr for the largest address in use but this would fail for example if done in Linux. So in theory one should use struct sockaddr_storage when storing the socket address, and struct sockaddr should only be used when passing socket information in functions.
Note that there is static inline struct sockaddr *net_sad(const struct sockaddr_storage *addr); to convert from storage to sockaddr.
Not sure how big change this would be, but might be good to change this if it is not too much work.
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.
Will look into that, thanks for pointing it out !
subsys/net/lib/midi2/netmidi2.c
Outdated
| static inline const struct netmidi2_user *netmidi2_find_user( | ||
| const struct netmidi2_ep *ep, const char *name, size_t namelen | ||
| ) |
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.
Indentation and formatting looks different than the other functions, perhaps change to
static inline const struct netmidi2_user *netmidi2_find_user(const struct netmidi2_ep *ep,
const char *name,
size_t namelen)
Note that there are other functions in this file that look a bit non-zephyr like indentation wise too.
3fcca35 to
480eca4
Compare
|
Thanks for your latest review @jukkar ! I just rebased on top of the latest main, applying the following changes:
|
|
@titouanc can you rebase to latest main and force push in order to get rid of CI failure |
Improve the API documentation for Universal MIDI Packets definitions: - Add link to the reference document, and make it referenceable in doxygen - Use BIT_MASK macro instead of hexadecimal litterals to better convey the length of various fields within UMP packets - Add more cross-references where possible - Use @remark when applicable Signed-off-by: Titouan Christophe <[email protected]>
Add a new top-level, transport independent library to respond to UMP Stream Discovery messages. This allows MIDI2.0 clients to discover UMP endpoints hosted on Zephyr over the UMP protocol. The endpoint specification can be gathered from the device tree, so that the same information used to generate USB descriptors in usb-midi2.0 can be delivered over UMP Stream. Signed-off-by: Titouan Christophe <[email protected]>
Since a new device-tree property has been introduced to describe if a
UMP group is limited to 31.25kb/s (like a physical MIDI DIN-5 port),
this can be used in the USB UMP group terminal block descriptors, as
specified in the USB-MIDI2.0 reference document [1], (in 5.4.2.1
Group Terminal Block Descriptor):
Maximum Output Bandwidth Capability in 4KB/second. 0x0000 – 0xFFFF.
Bandwidth is total for this Block, shared among all OUT Group Terminals.
0x0000 = Unknown or Not Fixed.
0x0001 = Rounded version of 31.25kb/s
[1] https://www.usb.org/sites/default/files/USB%20MIDI%20v2_0.pdf
Signed-off-by: Titouan Christophe <[email protected]>
Update the USB-MIDI2.0 sample to use the newly introduced UMP Stream Responder library. This allows the host to discover the topology and function block names (if supported by the host OS). Signed-off-by: Titouan Christophe <[email protected]>
Add a new network protocol for MIDI2.0 over the network, using UDP sockets. This allows Zephyr to host a UMP endpoint on the network, which can be invited by UMP clients to exchange MIDI2.0 data. Signed-off-by: Titouan Christophe <[email protected]>
Add a new very simple shield that provides MIDI DIN-5 IN/OUT/THROUGH, as well as 2 leds. Other features of the shields (capacitive sensors) are currently not reified in Zephyr. While there is no further device on the shield itself (everything goes through the Arduino header directly), this allows for meaningful device-tree naming when using the shield. Signed-off-by: Titouan Christophe <[email protected]>
Add a new sample to demonstrate usage of the newly introduced Network MIDI 2.0 host stack. Signed-off-by: Titouan Christophe <[email protected]>
|
jukkar
left a comment
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.
+1 to the networking changes
kartben
left a comment
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.
Refreshing earlier approval :)
|
Hi @titouanc |
|
Hello @butok looking at this now. |
Unfortunately, Zephyr CI is not reliable; it repeatedly does not catch errors |



Add a new Network MIDI2.0 Host stack, allowing Zephyr to expose a Universal MIDI Packet endpoint over UDP, so that clients can connect and exchange MIDI data over the network. The host optionally supports authentication with a single shared secret, or with a list of user/password.
This initial implementation is based on the following document:
User Datagram Protocol for Universal MIDI Packets - Network MIDI 2.0 (UDP) Transport Specification - Document version 1.0
In addition to the new network protocol, other related changes are shipped in this PR:
(To the best of my knowledge, this would make Zephyr the first OS with mainline support for Network MIDI 2.0)