-
Notifications
You must be signed in to change notification settings - Fork 2k
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
gcoap: Finish the gcoap_get_resource_list_tl -> gcoap_get_resource_list renaming #19295
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrysn
added
Process: API change
Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Area: CoAP
Area: Constrained Application Protocol implementations
labels
Feb 21, 2023
chrysn
requested review from
miri64,
haukepetersen,
PeterKietzmann and
cgundogan
as code owners
February 21, 2023 19:21
github-actions
bot
added
Area: build system
Area: Build system
Area: network
Area: Networking
Area: sys
Area: System
Area: tests
Area: tests and testing framework
labels
Feb 21, 2023
My apologies, this is based on the wrong branch, and thus not as trivial as it should have been. Expect a rebased version force-pushed momentarily. |
This is an API change in the latter, which would typically now take an extra argument GCOAP_SOCKET_TYPE_UNDEF. Follow-Up-For: RIOT-OS#16688
chrysn
force-pushed
the
followup-16688
branch
from
February 21, 2023 19:22
15f7cc3
to
86e6898
Compare
chrysn
added
the
CI: ready for build
If set, CI server will compile all applications for all available boards for the labeled PR
label
Feb 21, 2023
benpicco
approved these changes
Feb 21, 2023
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.
bors merge
bors cancel |
Canceled. |
bors merge |
bors bot
added a commit
that referenced
this pull request
Feb 21, 2023
18392: drivers/servo: reimplement with high level interface r=benpicco a=maribu ### Contribution description The previous servo driver didn't provide any benefit over using PWM directly, as users controlled the servo in terms of PWM duty cycles. This changes the interface to provide a high level interface that abstracts the gory PWM details. In addition, a SAUL layer and auto-initialization is provided. ### Testing procedure The test application provides access to the servo driver via the `saul` shell command. ``` > saul 2022-08-02 22:12:31,826 # saul 2022-08-02 22:12:31,827 # ID Class Name 2022-08-02 22:12:31,830 # #0 ACT_SWITCH LD1(green) 2022-08-02 22:12:31,832 # #1 ACT_SWITCH LD2(blue) 2022-08-02 22:12:31,834 # #2 ACT_SWITCH LD3(red) 2022-08-02 22:12:31,837 # #3 SENSE_BTN B1(User button) 2022-08-02 22:12:31,838 # #4 ACT_SERVO servo > saul write 4 0 2022-08-02 22:12:41,443 # saul write 4 0 2022-08-02 22:12:41,445 # Writing to device #4 - servo 2022-08-02 22:12:41,447 # Data: 0 2022-08-02 22:12:41,450 # [servo] setting 0 to 2949 (0 / 255) 2022-08-02 22:12:41,453 # data successfully written to device #4 > saul write 4 256 2022-08-02 22:12:45,343 # saul write 4 256 2022-08-02 22:12:45,346 # Writing to device #4 - servo 2022-08-02 22:12:45,347 # Data: 256 2022-08-02 22:12:45,351 # [servo] setting 0 to 6865 (255 / 255) 2022-08-02 22:12:45,354 # data successfully written to device #4 ``` Each write resulted in the MG90S servo that I connected to move to the corresponding position. ### Issues/PRs references 19292: sys/phydat: Fix unit confusion r=benpicco a=maribu ### Contribution description Previously, `UNIT_G` was used for g-force with the correct symbol `g`, `UNIT_GR` for gram (as in kilogram) with the incorrect symbol `G` (which would be correct for Gauss), and `UNIT_GS` for Gauss with symbol `Gs` (which is an alternative correct symbol). To avoid confusion between G-Force, Gauss, and Gram the units have been renamed to `UNIT_G_FORCE`, `UNIT_GRAM`, and `UNIT_GAUSS`. In addition, gram now uses the correct symbol `g`; which sadly is the same as for g-force. But usually there is enough context to tell them apart. ### Testing procedure Green CI ### Issues/PRs references None 19294: sys/shell: don't include suit command by default r=benpicco a=benpicco 19295: gcoap: Finish the gcoap_get_resource_list_tl -> gcoap_get_resource_list renaming r=benpicco a=chrysn ### Contribution description In #16688, an argument was added to the `gcoap_get_resource_list` function by creating a new function `gcoap_get_resource_list_tl` with a deprecation and roll-over plan. This plan has not been acted on so far. This PR shortens the original plan by just adding the argument to `gcoap_get_resource_list` and removing `gcoap_get_resource_list_tl` in a single go. The rationale for this deviation is that while it's a public API, its only two practical consumers are the (built-in) well-known/core implementation, and the (built-in) CoRE Resource Directory (cord) endpoint. Moreover, a further change to this API (switching over to `coap_block_slicer_t`) is expected to happen within this release cycle, which would take something like 4 total releases to get through otherwise, which is unrealistic for an API that there are no known external users of. A second commit clean up ToDo items (in the changed function's documentation) that referred to a IETF draft that has long been abandoned by the CoRE WG. ### Testing procedure Plain inspection and CI passing should suffice. ### AOB There is a second analogous pair left over from #16688, `gcoap_req_send` / `gcoap_req_send_tl`. As that *is* expected to be used widely, I prefer not to mix these two concerns, and get the present one through without unnecessary hold-up. Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de> Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de> Co-authored-by: chrysn <chrysn@fsfe.org>
Build failed (retrying...): |
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Area: CoAP
Area: Constrained Application Protocol implementations
Area: network
Area: Networking
Area: sys
Area: System
Area: tests
Area: tests and testing framework
CI: ready for build
If set, CI server will compile all applications for all available boards for the labeled PR
Process: API change
Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
In #16688, an argument was added to the
gcoap_get_resource_list
function by creating a new functiongcoap_get_resource_list_tl
with a deprecation and roll-over plan.This plan has not been acted on so far.
This PR shortens the original plan by just adding the argument to
gcoap_get_resource_list
and removinggcoap_get_resource_list_tl
in a single go. The rationale for this deviation is that while it's a public API, its only two practical consumers are the (built-in) well-known/core implementation, and the (built-in) CoRE Resource Directory (cord) endpoint. Moreover, a further change to this API (switching over tocoap_block_slicer_t
) is expected to happen within this release cycle, which would take something like 4 total releases to get through otherwise, which is unrealistic for an API that there are no known external users of.A second commit clean up ToDo items (in the changed function's documentation) that referred to a IETF draft that has long been abandoned by the CoRE WG.
Testing procedure
Plain inspection and CI passing should suffice.
AOB
There is a second analogous pair left over from #16688,
gcoap_req_send
/gcoap_req_send_tl
. As that is expected to be used widely, I prefer not to mix these two concerns, and get the present one through without unnecessary hold-up.