drivers/motor_driver: rework motor driver by gdoffe · Pull Request #20429 · RIOT-OS/RIOT

gdoffe · 2024-02-24T00:21:07Z

Contribution description

The motor_driver device driver is developed as a periph driver and it should not.
Make this driver compliant with RIOT device driver development guide [1].
Also make some cleanups and fix some typos.

[1] https://doc.riot-os.org/driver-guide.html

Testing procedure

Build for native architecture:

make -C tests/drivers/motor_driver BOARD=native

Run the test:

$ tests/drivers/motor_driver/bin/native/tests_motor_driver.elf
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

ztimer_init(): ZTIMER_TIMER using periph timer 0, freq 1000000, width 32
ztimer_init(): ZTIMER_MSEC convert_frac from 1000000 to 1000
main(): This is RIOT! (Version: 2024.04-devel-392-g6fff3-rework_motor_driver)

RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

ztimer_init(): ZTIMER_TIMER using periph timer 0, freq 1000000, width 32
ztimer_init(): ZTIMER_MSEC convert_frac from 1000000 to 1000
main(): This is RIOT! (Version: 2024.04-devel-392-g6fff3-rework_motor_driver)

Brake motors

Actuate Motors
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 0    PWM_VALUE = 50
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 1    PWM_VALUE = 50

Disable motors
Enable GPIO is not valid for motor 0, skipping disable
Enable GPIO is not valid for motor 1, skipping disable

Enable motors
Enable GPIO is not valid for motor 0, skipping enable
Enable GPIO is not valid for motor 1, skipping enable

Actuate Motors
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 0    PWM_VALUE = 100
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 1    PWM_VALUE = 100

Brake motors

Actuate Motors
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 0    PWM_VALUE = -50
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 1    PWM_VALUE = -50

Disable motors
Enable GPIO is not valid for motor 0, skipping disable
Enable GPIO is not valid for motor 1, skipping disable

Enable motors
Enable GPIO is not valid for motor 0, skipping enable
Enable GPIO is not valid for motor 1, skipping enable

Actuate Motors
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 0    PWM_VALUE = -100
MOTOR-DRIVER=0x80550e4    MOTOR_ID = 1    PWM_VALUE = -100

drivers/include/motor_driver.h

tests/drivers/motor_driver/main.c

kfessel · 2024-03-18T19:55:29Z

drivers/motor_driver/motor_driver.c

+    if ((gpio_is_valid(motor->gpio_dir0))
+       && (gpio_is_valid(motor->gpio_dir1_or_brake))) {
+        gpio_write(motor->gpio_dir0, direction);
+        gpio_write(motor->gpio_dir1_or_brake, direction ^ 0x1);


the XOR 0x01 seems a little non generic

not sure to understand, xor is not ok or 0x01 ?
I replaced with: direction ^ 1

drivers/include/motor_driver.h

tests/drivers/motor_driver/main.c

kfessel · 2024-03-18T20:18:24Z

drivers/motor_driver/motor_driver.c


-    return 0;
+    /* Set callbacks according to driver type */
+    switch(motor_driver_params->mode) {


wouldn't it make more sense do a switch when the brake or direction function is called instead of storing an extra function pointer?

The first idea was to make this two callbacks configurables with motor_driver_params_t. However as the driver is able to support most of existing motor drivers, I did not find really relevant to expose this callbacks inside motor_driver_params_t.
But I kept them internally, because it allows less lines of code at the price of 2 pointers, indeed.

As I do not have a real opinion about this, I follow your review. 😉

drivers/include/motor_driver.h

gdoffe · 2024-03-24T22:03:38Z

@kfessel thx for review, I added 2 fixup commits related to your comments and few other small things. 😉

tests/drivers/motor_driver/Makefile

kfessel

some nitpicks by our static_tests

tests/drivers/motor_driver/main.c

drivers/motor_driver/motor_driver.c

drivers/include/motor_driver.h

kfessel · 2024-03-26T16:13:25Z

this has minor API changes

crasbe · 2025-10-16T10:11:55Z

@gdoffe are you still interested in getting this merged? I (unknowingly) did a subset of this PR, but yours is superior.

I rebased your PR to the current master branch and added minor fixups, you can check it out here: https://github.com/crasbe/RIOT/tree/rework_motor_driver

Edit: The merge conflict was caused by #21626, but I think this PR replaces that part of the code anyway.

gdoffe · 2025-10-17T12:27:11Z

Hi @crasbe ! I left a few outdated PRs that I should update, this one is one of them.
The driver is fully functional; I've been using it since I opened it. I'll take a look at your rebase soon and get back to you.
Thanks for digging it up. 😅

crasbe · 2025-10-17T12:39:10Z

Thank you. I just rebased my branch again after merging #21337 yesterday, so it should be good to go again.

gdoffe · 2025-10-17T21:57:19Z

@crasbe fixup applied, seems ok for native. I will do real tests next week, I cannot test before. But I dont worry. 😉

gdoffe · 2025-10-29T13:22:36Z

@crasbe I fixed the issue from merge queue Murdock CI. But is it different from the Murdock triggered by the merge request ? 🤔

crasbe · 2025-10-29T13:24:26Z

The quick build only builds a selection of boards and the merge builds all boards. Therefore not all issues show up in the quick build.

The specific error showed up with the Bitcraze Crazyflie, because that uses the motor_driver parameters.

boards/bitcraze-crazyflie21-main/include/motor_driver_params.h

gdoffe · 2025-10-29T13:49:17Z

Arf sorry I missed your suggestions, I already pushed fixes. 😉

Do you want me to apply them ?

crasbe · 2025-10-29T14:31:58Z

Do you want me to apply them ?

Don't worry about those, I made new suggestions about the indentation for the #define. See also our coding convention: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#indentation-of-preprocessor-directives

crasbe · 2025-10-29T17:32:53Z

You'll have to add a tests/drivers/motor_driver/Makefile.ci file with the following content:

BOARD_INSUFFICIENT_MEMORY := \
    arduino-duemilanove \
    arduino-nano \
    arduino-uno \
    atmega328p \
    atmega328p-xplained-mini \
    atmega8 \
    #

I ran make generate-Makefile.ci on skyleaf to generate it for you :)

The motor_driver device driver is developed as a periph driver and it should not. Make this driver compliant with RIOT device driver development guide [1]. Also make some cleanups and fix some typos. [1] https://doc.riot-os.org/driver-guide.html Signed-off-by: Gilles DOFFE <[email protected]> Co-authored-by: crasbe <[email protected]>

Rework the test as the motor_driver module has been reworked to be compliant with RIOT device driver development guide. Mainly keep same behavior and use a simpler callback to illustrate post motor_set() callback use. Signed-off-by: Gilles DOFFE <[email protected]> Co-authored-by: crasbe <[email protected]>

XTIMER_BACKOFF and XTIMER_ISR_BACKOFF were defined in periph_conf.h since 2016 for the legacy xtimer module, which required high backoff values (200) on native to prevent timer underflow issues. However, since the migration to ztimer, all code now uses MODULE_ZTIMER_XTIMER_COMPAT which includes ztimer/xtimer_compat.h before periph_conf.h. This means xtimer_compat.h always defines XTIMER_BACKOFF=1 first, and the periph_conf.h definitions are never used. Remove these unused definitions as they serve no purpose with the current ztimer-based implementation. Signed-off-by: Gilles DOFFE <[email protected]>

The motor_driver module has been reworked in a previous commit to be compliant with RIOT device driver guide. Thus declaration in board.h is no more needed and should not work anymore. Moreover the driver test was calling a callback specific to the native architecture to simulate the native QDEC periph driver according to motors speed. This is not relevant as a test should only test the feature it has been developed for. Signed-off-by: Gilles DOFFE <[email protected]>

The motor_driver module has been reworked in a previous commit to be compliant with RIOT device driver guide. Thus declaration in board.h is no more needed and should not work anymore. Signed-off-by: Gilles DOFFE <[email protected]>

Migrate motor driver configuration from inline definition in board.h to the new board-specific motor_driver_params.h header file. The old approach defined motor_driver_config[] directly in board.h and unconditionally included motor_driver.h, causing compilation failures for applications that do not use the motor_driver module. This allows the motor driver header to be included only when the module is actually used, while board-specific parameters take precedence over driver defaults due to include path ordering. Configuration migrated: - Driver 0: PWM device 1, 3 motors (channels 1, 3, 0) - Driver 1: PWM device 2, 1 motor (channel 0) - Mode: MOTOR_DRIVER_1_DIR with inverted brake for both drivers Signed-off-by: Gilles DOFFE <[email protected]>

Remove redundant macros since CONFIG_ZTIMER_USEC_TYPE and CONFIG_ZTIMER_USEC_DEV were already set to their default values. Signed-off-by: Gilles DOFFE <[email protected]>

crasbe · 2025-10-30T10:08:41Z

Thank you for pulling this through :)

gdoffe · 2025-10-30T10:24:53Z

Thank you for pulling this through :)

Thank to you for your help !

gdoffe requested review from MichelRottleuthner, aabadie and leandrolanzieri as code owners February 24, 2024 00:21

github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: boards Area: Board ports labels Feb 24, 2024

benpicco requested a review from maribu February 26, 2024 10:42

kfessel reviewed Mar 8, 2024

View reviewed changes

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

kfessel reviewed Mar 8, 2024

View reviewed changes

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

gdoffe force-pushed the rework_motor_driver branch from 43ad42f to f975209 Compare March 11, 2024 16:28

gdoffe requested a review from kfessel March 11, 2024 16:29

gdoffe force-pushed the rework_motor_driver branch from f975209 to 6c4dbb8 Compare March 14, 2024 22:08

kfessel reviewed Mar 18, 2024

View reviewed changes

drivers/include/motor_driver.h Show resolved Hide resolved

kfessel reviewed Mar 26, 2024

View reviewed changes

tests/drivers/motor_driver/Makefile Outdated Show resolved Hide resolved

kfessel reviewed Mar 26, 2024

View reviewed changes

kfessel added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Mar 26, 2024

gdoffe force-pushed the rework_motor_driver branch from 6fff369 to 3e5dec2 Compare April 5, 2024 23:43

crasbe mentioned this pull request Oct 16, 2025

drivers/motor_driver: make motor configuration non-global #21798

Closed

crasbe added the State: needs rebase State: The codebase was changed since the creation of the PR, making a rebase necessary label Oct 16, 2025

AnnsAnns mentioned this pull request Oct 16, 2025

boards/nucleo*: Cleanup and Enhance the Documentation #21337

Merged

14 tasks

gdoffe force-pushed the rework_motor_driver branch from 3e5dec2 to 342333e Compare October 17, 2025 21:51

github-actions bot added the Area: tools Area: Supplementary tools label Oct 17, 2025

crasbe added this pull request to the merge queue Oct 28, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2025

gdoffe force-pushed the rework_motor_driver branch from 58b731c to 3a30592 Compare October 29, 2025 13:21

crasbe reviewed Oct 29, 2025

View reviewed changes

gdoffe force-pushed the rework_motor_driver branch from 3a30592 to dd6701a Compare October 29, 2025 13:40

crasbe reviewed Oct 29, 2025

View reviewed changes

boards/bitcraze-crazyflie21-main/include/motor_driver_params.h Outdated Show resolved Hide resolved

boards/bitcraze-crazyflie21-main/include/motor_driver_params.h Outdated Show resolved Hide resolved

gdoffe force-pushed the rework_motor_driver branch from dd6701a to ac3b403 Compare October 29, 2025 16:03

crasbe enabled auto-merge October 29, 2025 16:05

crasbe added this pull request to the merge queue Oct 29, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 29, 2025

gdoffe and others added 7 commits October 30, 2025 08:20

boards/nucleo64: clean motor_driver old configuration

f33b509

The motor_driver module has been reworked in a previous commit to be compliant with RIOT device driver guide. Thus declaration in board.h is no more needed and should not work anymore. Signed-off-by: Gilles DOFFE <[email protected]>

boards/native: remove redundant ztimer configuration

22c6b2b

Remove redundant macros since CONFIG_ZTIMER_USEC_TYPE and CONFIG_ZTIMER_USEC_DEV were already set to their default values. Signed-off-by: Gilles DOFFE <[email protected]>

gdoffe force-pushed the rework_motor_driver branch from ac3b403 to 22c6b2b Compare October 30, 2025 07:20

crasbe added this pull request to the merge queue Oct 30, 2025

Merged via the queue into RIOT-OS:master with commit 49b01c4 Oct 30, 2025
25 checks passed

gdoffe deleted the rework_motor_driver branch October 30, 2025 09:58

benpicco added this to the Release 2025.10 milestone Dec 3, 2025

Conversation

gdoffe commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfessel Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

gdoffe Mar 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kfessel Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

gdoffe Mar 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gdoffe commented Mar 24, 2024

Uh oh!

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kfessel commented Mar 26, 2024

Uh oh!

crasbe commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gdoffe commented Oct 17, 2025

Uh oh!

crasbe commented Oct 17, 2025

Uh oh!

gdoffe commented Oct 17, 2025

Uh oh!

Uh oh!

gdoffe commented Oct 29, 2025

Uh oh!

crasbe commented Oct 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gdoffe commented Oct 29, 2025

Uh oh!

crasbe commented Oct 29, 2025

Uh oh!

Uh oh!

crasbe commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

crasbe commented Oct 30, 2025

Uh oh!

gdoffe commented Oct 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

gdoffe commented Feb 24, 2024 •

edited

Loading

crasbe commented Oct 16, 2025 •

edited

Loading

crasbe commented Oct 29, 2025 •

edited

Loading