drivers/servo: reimplement with high level interface #18392

maribu · 2022-08-02T20:34:40Z

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

This is a verbatim copy of the PWM config of `boards/nucleo-f746zg`. However, those boards are almost identical. I successfully tested the configuration via RIOT-OS#18392

maribu · 2022-08-04T13:10:22Z

I updated the code to calculate the duty cycle at runtime, as the actual PWM frequency is not known on compile time. E.g. on the Arduino UNO the actual PWM frequency is 61 Hz (if I recall correctly). With that, the actual extension and the requested one differs by 22 % if using the nominal frequency instead of the actual one.

(Yes, this works fine on the Arduino UNO when rebased on top of #18148 :))

benpicco

Yea that's a much better API.
I don't quite understand the reason for use of XFA - why not use the same pattern as the other drivers?

drivers/include/servo.h

drivers/servo/servo.c

maribu · 2022-10-18T07:16:48Z

I don't quite understand the reason for use of XFA - why not use the same pattern as the other drivers?

Likely I should have done the conversion as a follow up. Note that there are two things that are changed: First, it allocates device states and params as an arrays and uses an index has handle instead of a pointer. Second, it uses XFA to implement the arrays.

The use of arrays has the following advantages:

It safes RAM, as the current pattern of copying the params stuff into the state is no longer needed. (The alternative would be to add a pointer from the state to params, which would increase the state size by one pointer and add one level of pointer traversal to accessing params.)
And most importantly: It fixes the fundamentally broken sensor and actuator auto initialization.
- Auto-initialization does happen if used with SAUL, but then there is no way to access the device state and use it without the SAUL API.
- When not using SAUL, currently no way of auto-initialization is possible
- Using indexes solves the issue of obtaining the device pointer when initialization happens in auto_init

The use of XFA has the advantage that a user can easily add additional devices. E.g. let's say board foo already has 3 LEDs exposed via SAUL. Now we add additional LEDs via a shield. With XFA, we can just extend the LED array without having to mess with the board LED config.

Likely I should update an existing driver, maybe the SHT1x, to the XFA approach, first.

maribu · 2022-10-24T22:02:45Z

I dropped the use an of index-based servo_t and of XFA for consistency. The servo shell command had to be removed as well, as co-existence with SAUL no longer is possible.

This can be added in a follow up. Let's fix the servo API first.

benpicco

CI has some comments, otherwise looks good

riot-ci · 2022-10-25T14:02:37Z

Murdock results

✔️ PASSED

6dc2a60 drivers/servo: reimplement with high level interface

Success	Failures	Total	Runtime
6865	0	6865	13m:10s

Artifacts

Documentation preview

maribu · 2023-02-21T19:52:12Z

I forgot the KConfig integration and added it now.

benpicco · 2023-02-21T21:17:37Z

I forgot the KConfig integration and added it now.

pretty much impossible to get that right on the first try 😉

benpicco · 2023-02-21T21:48:42Z

bors merge

benpicco · 2023-02-21T23:00:32Z

bors cancel

bors · 2023-02-21T23:00:38Z

Canceled.

benpicco · 2023-02-21T23:01:08Z

bors merge

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 <[email protected]> Co-authored-by: Benjamin Valentin <[email protected]> Co-authored-by: chrysn <[email protected]>

bors · 2023-02-21T23:12:09Z

Build failed (retrying...):

Murdock

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 Co-authored-by: Marian Buschsieweke <[email protected]>

bors · 2023-02-21T23:14:09Z

This PR was included in a batch that was canceled, it will be automatically retried

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 Co-authored-by: Marian Buschsieweke <[email protected]>

bors · 2023-02-22T08:08:39Z

Build failed:

Murdock

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. Co-authored-by: benpicco <[email protected]>

maribu · 2023-02-22T09:00:44Z

bors retry

Now with a Makefile.ci, let's see what happens now.

bors · 2023-02-22T09:00:47Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

maribu · 2023-02-22T09:15:44Z

bors retry

bors · 2023-02-22T12:01:57Z

Build succeeded:

maribu · 2023-02-22T12:13:22Z

Thx :)

maribu requested review from smlng, leandrolanzieri, aabadie, MichelRottleuthner and fjmolinas as code owners August 2, 2022 20:34

github-actions bot added Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework labels Aug 2, 2022

maribu mentioned this pull request Aug 2, 2022

boards/nucleo-f767zi: add PWM config #18393

Merged

maribu added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Aug 2, 2022

benpicco requested review from haukepetersen and jnohlgard August 4, 2022 12:52

maribu force-pushed the drivers/servo branch from 8de05a1 to 237f1e0 Compare October 14, 2022 09:11

benpicco reviewed Oct 17, 2022

View reviewed changes

drivers/include/servo.h Outdated Show resolved Hide resolved

drivers/include/servo.h Outdated Show resolved Hide resolved

drivers/include/servo.h Outdated Show resolved Hide resolved

drivers/servo/servo.c Outdated Show resolved Hide resolved

maribu force-pushed the drivers/servo branch from 78d93d4 to 19606c0 Compare October 24, 2022 22:00

github-actions bot added Area: SAUL Area: Sensor/Actuator Uber Layer and removed Area: sys Area: System labels Oct 24, 2022

benpicco reviewed Oct 25, 2022

View reviewed changes

maribu force-pushed the drivers/servo branch from 19606c0 to d655730 Compare October 25, 2022 11:10

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 25, 2022

benpicco previously approved these changes Oct 25, 2022

View reviewed changes

maribu force-pushed the drivers/servo branch from d655730 to 3a174a4 Compare October 26, 2022 20:38

github-actions bot added the Area: build system Area: Build system label Oct 26, 2022

benpicco 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

maribu force-pushed the drivers/servo branch from 7d767d5 to c22a210 Compare February 21, 2023 19:50

maribu requested review from jia200x and MrKevinWeiss as code owners February 21, 2023 19:50

github-actions bot added the Area: Kconfig Area: Kconfig integration label Feb 21, 2023

maribu force-pushed the drivers/servo branch from c22a210 to e8eca6d Compare February 21, 2023 19:51

maribu force-pushed the drivers/servo branch from e8eca6d to 0ac32fa Compare February 21, 2023 21:05

maribu force-pushed the drivers/servo branch from 0ac32fa to 463e2e7 Compare February 21, 2023 21:33

maribu force-pushed the drivers/servo branch from 463e2e7 to 6dc2a60 Compare February 22, 2023 09:00

bors bot merged commit 41b54d8 into RIOT-OS:master Feb 22, 2023

maribu deleted the drivers/servo branch February 22, 2023 12:12

MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/servo: reimplement with high level interface #18392

drivers/servo: reimplement with high level interface #18392

maribu commented Aug 2, 2022 •

edited

Loading

maribu commented Aug 4, 2022

benpicco left a comment •

edited

Loading

maribu commented Oct 18, 2022

maribu commented Oct 24, 2022

benpicco left a comment

riot-ci commented Oct 25, 2022 •

edited

Loading

maribu commented Feb 21, 2023

benpicco commented Feb 21, 2023

benpicco commented Feb 21, 2023

benpicco commented Feb 21, 2023

bors bot commented Feb 21, 2023

benpicco commented Feb 21, 2023

bors bot commented Feb 21, 2023

bors bot commented Feb 21, 2023

bors bot commented Feb 22, 2023

maribu commented Feb 22, 2023

bors bot commented Feb 22, 2023

maribu commented Feb 22, 2023

bors bot commented Feb 22, 2023

maribu commented Feb 22, 2023

drivers/servo: reimplement with high level interface #18392

drivers/servo: reimplement with high level interface #18392

Conversation

maribu commented Aug 2, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

maribu commented Aug 4, 2022

benpicco left a comment • edited Loading

Choose a reason for hiding this comment

maribu commented Oct 18, 2022

maribu commented Oct 24, 2022

benpicco left a comment

Choose a reason for hiding this comment

riot-ci commented Oct 25, 2022 • edited Loading

Murdock results

Artifacts

maribu commented Feb 21, 2023

benpicco commented Feb 21, 2023

benpicco commented Feb 21, 2023

benpicco commented Feb 21, 2023

bors bot commented Feb 21, 2023

benpicco commented Feb 21, 2023

bors bot commented Feb 21, 2023

bors bot commented Feb 21, 2023

bors bot commented Feb 22, 2023

maribu commented Feb 22, 2023

bors bot commented Feb 22, 2023

maribu commented Feb 22, 2023

bors bot commented Feb 22, 2023

maribu commented Feb 22, 2023

maribu commented Aug 2, 2022 •

edited

Loading

benpicco left a comment •

edited

Loading

riot-ci commented Oct 25, 2022 •

edited

Loading