drivers/motor_driver: rework motor driver #20429

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

kfessel · 2024-03-18T19:37:14Z

tests/drivers/motor_driver/main.c

 /* set interval to 20 milli-second */
 #define INTERVAL (3000 * US_PER_MS)


something does not match

kfessel · 2024-03-18T19:38:08Z

tests/drivers/motor_driver/main.c

+        puts("\nDisable motors");
+        motor_disable(&motor_driver, MOTOR_0_ID);
+        motor_disable(&motor_driver, MOTOR_1_ID);
+        xtimer_periodic_wakeup(&last_wakeup, INTERVAL);


port to ztimer?

good catch, done.

kfessel · 2024-03-18T19:38:39Z

tests/drivers/motor_driver/main.c

@@ -72,9 +88,9 @@ void motion_control(void)
    int8_t dir = 1;
    int ret = 0;
    xtimer_ticks32_t last_wakeup /*, start*/;


Suggested change

xtimer_ticks32_t last_wakeup /*, start*/;

xtimer_ticks32_t last_wakeup;

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

kfessel · 2024-03-18T20:07:49Z

drivers/include/motor_driver.h

@@ -145,59 +135,81 @@ typedef struct {
    gpio_t gpio_enable;         /**< GPIO to enable/disable motor */
    gpio_t gpio_dir0;           /**< GPIO to control rotation direction */
    gpio_t gpio_dir1_or_brake;  /**< GPIO to control rotation direction */


Suggested change

gpio_t gpio_dir1_or_brake; /**< GPIO to control rotation direction */

union {

gpio_t gpio_dir1; /**< GPIO to control rotation direction */

gpio_t gpio_breake; /**< GPIO to break */

}

kfessel · 2024-03-18T20:17:16Z

tests/drivers/motor_driver/main.c

 #include "log.h"
 #include "motor_driver.h"
+#include "motor_driver_params.h"
 #include "test_utils/expect.h"
 #include "xtimer.h"


Suggested change

#include "xtimer.h"

#include "ztimer.h"

if you like

did you push your changes?

just pushed the fixes, was off for few days, sorry. ;)

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. 😉

kfessel · 2024-03-26T16:03:49Z

tests/drivers/motor_driver/Makefile

@@ -6,7 +6,8 @@ include ../Makefile.drivers_common

 USEMODULE += motor_driver
 USEMODULE += shell_cmds_default
-USEMODULE += xtimer
+USEMODULE += ztimer
+USEMODULE += ztimer_sec


msec might be a better choice - every timer might be off by one count for second -> 3-4 seconds for msec 3000 - 3001 milliseconds

( usually it is a good choice to not wait single digits if you care about the time you wait)

kfessel

some nitpicks by our static_tests

kfessel · 2024-03-26T16:05:38Z

tests/drivers/motor_driver/main.c

    if (ret) {
        LOG_ERROR("motor_driver_init failed with error code %d\n", ret);
    }
    expect(ret == 0);

-    for (;;) {
+    while(1) {


Suggested change

while(1) {

while (1) {

kfessel · 2024-03-26T16:05:50Z

tests/drivers/motor_driver/main.c


-/* set interval to 20 milli-second */
-#define INTERVAL (3000 * US_PER_MS)
+


Suggested change

kfessel · 2024-03-26T16:06:17Z

drivers/motor_driver/motor_driver.c

+    int state = irq_disable();
+
+    /* Set direction */
+    switch(motor_driver->params->mode) {


Suggested change

switch(motor_driver->params->mode) {

switch (motor_driver->params->mode) {

kfessel · 2024-03-26T16:06:30Z

drivers/motor_driver/motor_driver.c

-    if (cb) {
-        cb(motor_driver, motor_id, pwm_duty_cycle);
+    /* Remove brake */
+    switch(motor_driver->params->mode) {


Suggested change

switch(motor_driver->params->mode) {

switch (motor_driver->params->mode) {

kfessel · 2024-03-26T16:06:41Z

drivers/motor_driver/motor_driver.c

+    int state = irq_disable();
+
+    /* Apply brake */
+    switch(motor_driver->params->mode) {


Suggested change

switch(motor_driver->params->mode) {

switch (motor_driver->params->mode) {

kfessel · 2024-03-26T16:10:01Z

drivers/motor_driver/motor_driver.c

-    pwm_mode_t mode = motor_driver_conf->pwm_mode;
-    uint32_t freq = motor_driver_conf->pwm_frequency;
-    uint16_t resol = motor_driver_conf->pwm_resolution;
+int motor_driver_init(motor_driver_t *motor_driver, const motor_driver_params_t *motor_driver_params)


Suggested change

int motor_driver_init(motor_driver_t *motor_driver, const motor_driver_params_t *motor_driver_params)

int motor_driver_init(motor_driver_t *motor_driver,

const motor_driver_params_t *motor_driver_params)

or may be you can shorten one of the params names

Suggested change

int motor_driver_init(motor_driver_t *motor_driver, const motor_driver_params_t *motor_driver_params)

int motor_driver_init(motor_driver_t *motor_driver, const motor_driver_params_t *params)

kfessel · 2024-03-26T16:10:48Z

drivers/include/motor_driver.h

 */
-int motor_driver_init(const motor_driver_t motor_driver);
+int motor_driver_init(motor_driver_t *motor_driver, const motor_driver_params_t *motor_driver_params);


I think you don't need > 100 chars for this

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

this has minor API changes

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]>

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]>

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]>

gdoffe requested review from leandrolanzieri, aabadie and MichelRottleuthner 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

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 added 4 commits April 6, 2024 01:42

boards/nucleo64: clean motor_driver old configuration

5d957ee

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]>

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

		/* set interval to 20 milli-second */
		#define INTERVAL (3000 * US_PER_MS)

	xtimer_ticks32_t last_wakeup /, start/;
	xtimer_ticks32_t last_wakeup;

-    gpio_t gpio_dir1_or_brake;  /**< GPIO to control rotation direction */
+    union {
+        gpio_t gpio_dir1;       /**< GPIO to control rotation direction */
+        gpio_t gpio_breake;     /**< GPIO to break */
+    }

	switch(motor_driver->params->mode) {
	switch (motor_driver->params->mode) {

	int motor_driver_init(motor_driver_t motor_driver, const motor_driver_params_t motor_driver_params)
	int motor_driver_init(motor_driver_t *motor_driver,
	const motor_driver_params_t *motor_driver_params)

drivers/motor_driver: rework motor driver #20429

Are you sure you want to change the base?

drivers/motor_driver: rework motor driver #20429

Conversation

gdoffe commented Feb 24, 2024 • edited Loading

Contribution description

Testing procedure

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdoffe commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfessel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfessel commented Mar 26, 2024

gdoffe commented Feb 24, 2024 •

edited

Loading