-
Notifications
You must be signed in to change notification settings - Fork 2.1k
drivers/slipmux: Rework SLIPDEV driver in preparation for Unicoap #21907
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
base: master
Are you sure you want to change the base?
Conversation
| while (1) { | ||
| event_t *event = event_wait(&queue); | ||
| event->handler(event); | ||
| } |
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 whole event queue contraption is rather odd but it will fit nicely with unicoap, trust me bro.
Murdock results❌ FAILED 27af9a1 unfinished Kconf Build failures (14)
Artifacts |
crasbe
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.
Just some basic style and nitpick comments for now.
Also the static test has some ideas and you sometimes added yourself in the Copyright and sometimes in the Doxygen authorship, but not really consistently yet 🤔
| PSEUDOMODULES += slipdev_stdio | ||
| PSEUDOMODULES += slipdev_config | ||
| PSEUDOMODULES += slipdev_l2addr |
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.
We don't need these modules as slipmux_ PSEUDOMODULES anymore?
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.
We still do! And this actually one of the major reasons for this rewrite/refactoring: The ability to select _stdio or _coap (used to be _config) without having to use _net (used to not exist and was always included in the base slipdev)
The definitions moved into Makefile.include. I can't remember why I decided to move them out of the global pseudomodules.inc.mk. Either a mistake or some deeper reasoning? Will see in a bit.
Way too early :p but thanks anyways, I adopted the fixes.
Probably. I wasn't done-enough yet, to look at them.
No care was given to that yet. |
9e542dd to
33e529a
Compare
|
I think this is ready for actual review now. I would appreciate feedback specifically on the mutex handling. I am not super proud of it. |
|
If you want to, you can squash before the reviews start pouring in 🤔 |
a64fda5 to
25826ef
Compare
mguetschow
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.
Great stuff! ❤️ Unfortunately it was a bit hard to review since Github didn't properly display the file renames (separate commit would have helped).
I've got mostly nitpicky things and haven't tested this yet.
Also Murdock fails because you probably have to change the Kconfig include from slipdev to slipmux somewhere.
| #if IS_USED(MODULE_SLIPMUX_NET_L2ADDR) | ||
| case NETOPT_ADDRESS_LONG: | ||
| assert(max_len == sizeof(eui64_t)); | ||
| netdev_eui64_get(netdev, value); | ||
| return sizeof(eui64_t); | ||
| #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.
What about this is slipmux-specific? why not use a generic l2addr module? I have the feeling there was some, maybe I'm wrong.
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 idea, I honestly didn't question it. It works tho! Tested it! :P
| /drivers/si70xx/ @basilfx | ||
| /drivers/slipdev/ @miri64 | ||
| /drivers/sx127x/ @aabadie @jia200x |
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.
| /drivers/si70xx/ @basilfx | |
| /drivers/slipdev/ @miri64 | |
| /drivers/sx127x/ @aabadie @jia200x | |
| /drivers/si70xx/ @basilfx | |
| /drivers/slipmux/ @Teufelchen1 | |
| /drivers/sx127x/ @aabadie @jia200x |
:P
mguetschow
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.
Forgot the most important file :P
| if (len <= 0) { | ||
| return; |
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.
some DEBUG messages wouldn't harm :)
|
Btw, while editing the title I was thinking: does this really belong to |
Thanks!
Yes, I realized too late that this approach was shit (sorry!).
I don't know. 🙈 |
Contribution description
Hey hey 🦭
This refactors the current
slipdevdriver heavily and also renames it toslipmux.The networking aspect (plain
slip) is no longer the default but a submodule:slipmux_net.The "configuration" aspect (coap via uart) is no longer called
slipdev_configbutslipmux_coap. That moves it further from the slipmux draft but (I hope) closer to the average developer and sets the expectations clearer.Same for diagnostic but there it was already called
_stdiowhich was retained.There is a new "overall" module called
slipmuxwhich enables all three modes (slipmux_stdio,_coapand_net) for full slipmux functionality.The parser behavior got a slight change and differentiates between unknown frame types and IPv4/6 frames now. This should make it more robust when interfacing slipmux instead of slip. (Worry not, our slip tools still work as expected)
In addition, the coap handling got reworked to be ready for plug & play action with unicoap once #21582 is merged.
Testing procedure
Pick one of our examples that offer slip e.g.
examples/networking/gnrc/border_router/and see that it still behaves as advertised in the README. tl;dr:See the
slipdevthread running (pid 2) and then try to ping the border router from your linux console: