drivers/slipdev: Replace nanocoap with unicoap - #22491
Conversation
72cf4bc to
97fe3e5
Compare
8f371a1 to
506523c
Compare
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_dtls for CoAP over DTLS driver) | ||
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_udp for CoAP over UDP driver) | ||
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_rfc7252_pdu for CoAP over UDP/DTLS PDU parser only) | ||
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_slipmux for CoAP over Slipmux driver) |
There was a problem hiding this comment.
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_dtls for CoAP over DTLS driver) | |
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_udp for CoAP over UDP driver) | |
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_rfc7252_pdu for CoAP over UDP/DTLS PDU parser only) | |
| $(info +++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_slipmux for CoAP over Slipmux driver) | |
| $(shell echo '+++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_dtls for CoAP over DTLS driver' 1>&2) | |
| $(shell echo '+++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_udp for CoAP over UDP driver' 1>&2) | |
| $(shell echo '+++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_rfc7252_pdu for CoAP over UDP/DTLS PDU parser only' 1>&2) | |
| $(shell echo '+++ FIXIT: Makefile: Add USEMODULE += unicoap_driver_slipmux for CoAP over Slipmux driver' 1>&2) |
For info messages that are printed during the build process, it's a good idea to print them to stderr to avoid them interfering with our CI scripts.
There was a problem hiding this comment.
I am not sure I agree here. So far, the pattern you are proposing has only been used a single time in RIOT (board_alias.inc.mk:30), but just using info is well established.
There was a problem hiding this comment.
That's not true, I had to use it for #22355 and there are many places that use $(COLOR_ECHO) instead:
cbuec@W11nMate:~/RIOTstuff/riot-vanillaice/RIOT$ grep -RnwI "1>&2" . | grep Makefile
./Makefile.include:212: "https://github.com/RIOT-OS/RIOT/blob/master/dist/tools/vagrant/README.md 1>&2)
./Makefile.include:974: "$(sort $(filter $(DISABLE_MODULE), $(USEMODULE)))" 1>&2)
./Makefile.include:982: "$(FEATURES_MISSING)" 1>&2)
./Makefile.include:989: "$(FEATURES_USED_BLACKLISTED)" 1>&2)
./Makefile.include:998: "BOARDS_SUPPORTED and BOARD_BLACKLIST to BOARDS_UNSUPPORTED.$(COLOR_RESET)" 1>&2)
./Makefile.include:1000: "BOARDS_SUPPORTED and the contents of BOARD_BLACKLIST to BOARDS_UNSUPPORTED for compatibility." 1>&2)
./Makefile.include:1006: "$(FEATURES_CONFLICTING)" 1>&2)
./Makefile.include:1008: $(shell $(COLOR_ECHO) "$(COLOR_YELLOW)Rationale: $(COLOR_RESET)$(FEATURES_CONFLICT_MSG)" 1>&2)
./Makefile.include:1015: $(shell $(COLOR_ECHO) "$(COLOR_RED)The selected BOARD=$(BOARD) is not listed in BOARDS_SUPPORTED$(COLOR_RESET)." 1>&2)
./Makefile.include:1016: $(shell $(COLOR_ECHO) "$(COLOR_YELLOW)When the list BOARDS_SUPPORTED is not empty, only boards explicitly listed there are supported.$(COLOR_RESET)" 1>&2)
./Makefile.include:1017: $(shell $(COLOR_ECHO) "BOARDS_SUPPORTED = \"$(BOARDS_SUPPORTED)\"" 1>&2)
./Makefile.include:1024: $(shell $(COLOR_ECHO) "$(COLOR_RED)The selected BOARD=$(BOARD) is listed in BOARDS_UNSUPPORTED.$(COLOR_RESET)" 1>&2)
./Makefile.include:1025: $(shell $(COLOR_ECHO) "BOARDS_UNSUPPORTED = $(BOARDS_UNSUPPORTED)" 1>&2)
./Makefile.include:1031: $(shell $(COLOR_ECHO) "$(COLOR_RED)The selected TOOLCHAIN=$(TOOLCHAIN) is not supported.$(COLOR_RESET)\nSupported toolchains: $(TOOLCHAINS_SUPPORTED)" 1>&2)
./Makefile.include:1038: $(shell $(COLOR_ECHO) "$(COLOR_RED)The selected TOOLCHAIN=$(TOOLCHAIN) is blacklisted:$(COLOR_RESET) $(TOOLCHAINS_BLACKLIST)" 1>&2)
./Makefile.include:1049: $(shell $(COLOR_ECHO) "\n\n$(COLOR_RED)EXPECT ERRORS!$(COLOR_RESET)\n\n" 1>&2)
./boards/seeedstudio-xiao-esp32c3/Makefile.dep:8: $(shell $(COLOR_ECHO) "$(COLOR_YELLOW)$(MSG)$(COLOR_RESET)" 1>&2)
./boards/common/nucleo64/Makefile.dep:13: $(shell $(COLOR_ECHO) "$(COLOR_YELLOW)$(MSG)$(COLOR_RESET)" 1>&2)
./sys/shell_lock/Makefile.include:4: used connection method!$(COLOR_RESET)" 1>&2)
./sys/net/application_layer/gcoap/Makefile.include:3: possible to use a dual stack setup$(COLOR_RESET)" 1>&2)
cbuec@W11nMate:~/RIOTstuff/riot-vanillaice/RIOT$ grep -RnwI "1>&2" . | grep ".inc.mk"
./makefiles/deprecated_cpus.inc.mk:6: "$(CPU)" 1>&2)
./makefiles/usb-codes.inc.mk:23: $(COLOR_ECHO) "$(COLOR_RED)Private testing pid.codes USB VID/PID used!, do not use it outside of test environments!$(COLOR_RESET)" 1>&2 ; \
./makefiles/usb-codes.inc.mk:24: $(COLOR_ECHO) "$(COLOR_RED)MUST NOT be used on any device redistributed, sold or manufactured, VID/PID is not unique!$(COLOR_RESET)" 1>&2 ; \
./makefiles/usb-codes.inc.mk:27: $(COLOR_ECHO) "$(COLOR_RED)RIOT standard peripherals code (1209/7D00) cannot be set explicitly.$(COLOR_RESET)" 1>&2 ; \
./makefiles/usb-codes.inc.mk:28: $(COLOR_ECHO) "$(COLOR_RED)Unset USB_VID / USB_PID for the code to be picked automatically, or set$(COLOR_RESET)" 1>&2 ; \
./makefiles/usb-codes.inc.mk:29: $(COLOR_ECHO) "$(COLOR_RED)them to \$${USB_VID_TESTING} / \$${USB_PID_TESTING} during development.$(COLOR_RESET)" 1>&2 ; \
./makefiles/deprecated_boards.inc.mk:7: "$(BOARD)" 1>&2)
./makefiles/dependency_resolution.inc.mk:72: "$(DEPRECATED_MODULES_USED)" 1>&2)
./makefiles/dependency_resolution.inc.mk:104: "is a REALLY BAD idea before proceeding!$(COLOR_RESET)" 1>&2)
./makefiles/dependency_resolution.inc.mk:108: "don't run this on public networks!$(COLOR_RESET)" 1>&2)
./makefiles/dependency_resolution.inc.mk:117: "is a REALLY BAD idea before proceeding!$(COLOR_RESET)" 1>&2)
./makefiles/dependency_resolution.inc.mk:121: "don't run this on public networks!$(COLOR_RESET)" 1>&2)
./makefiles/dependency_resolution.inc.mk:130: "security issues!$(COLOR_RESET)" 1>&2)
./makefiles/board_alias.inc.mk:30: $(shell echo 'using BOARD="$(_board)" as "$(_alias)" on a $(_platform_bits)-bit system' 1>&2)
./makefiles/board_alias.inc.mk:33: $(shell $(COLOR_ECHO) "$(COLOR_RED)$(MSG)$(COLOR_RESET)" 1>&2)
There was a problem hiding this comment.
I guess my "\$\(shell echo .*' 1>&2\)" (regex, case sensitive) was faulty, sorry!
The flip side, of frequent usage of info without redirect to stderr remains though, we should clean that up.
There was a problem hiding this comment.
I wonder if it wouldn't be nice to have $(call stderr, "blabla white test"), $(call stderr_red, "blabla red text"), instead of $(shell $(COLOR_ECHO "$(COLOR_RED)blabla red text$(COLOR_RESET)" 1>&2).
But we should probably keep track of that in a separate issue.
There was a problem hiding this comment.
The info thing is not supposed to be an error. a warning is a warning and not an error.
There was a problem hiding this comment.
but whether warnings are supposed to go to stderr, idk
There was a problem hiding this comment.
just make sure nothing gets treated as a hard error here.
There was a problem hiding this comment.
for future reference, I oppose -Werror.
|
From a functionality standpoint, we can merge this because I'm sure you'll use it a lot in the future, so any potential issues will be discovered (and fixed I hope). |
Yes, I found issues today. Only minor stuff. Other than that, works great! I also made sure to test the protocol exclusive access to resources - worked on the first try! |
| @@ -0,0 +1,20 @@ | |||
| @defgroup net_unicoap_drivers_slipmux CoAP over Slipmux Driver | |||
There was a problem hiding this comment.
Is slipmux really RFC 7252? But maybe I am misunderstanding the structure here.
There was a problem hiding this comment.
no it is not but it is Configuration is performed by sending CoAP messages [[RFC7252](https://datatracker.ietf.org/doc/html/rfc7252)] in SLIP framing.. So code-wise it makes a lot of sense to stick it next to coap over udp, as it is very very similar looking.
There was a problem hiding this comment.
if there's even the slightest change in framing, this warrants a new driver IMO. And I'm correct in assuming slipmux over 7252-like PDUs is not RIOT-specific, am I not?
There was a problem hiding this comment.
No it is not RIOT specific. What solution would make you happy?
bfc8555 to
2763dc7
Compare
Co-authored-by: crasbe <crasbe@gmail.com>
2763dc7 to
9663dd3
Compare
Contribution description
Hey hey 🐆
this reworks the slipdev driver to utilize the new unicoap server instead of nanocoap.
Reason: Unicoap has fewer dependencies which means small binaries in a lot of use cases (in particular: Using
slipdev_configwithout any IP networking is now significantly cheaper!). We also gain all the other benefits unicoap offers :pTesting procedure
Review, compile and manual testing
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: