periph/pwm: support for PWM extension API by gschorcht · Pull Request #10533 · RIOT-OS/RIOT

gschorcht · 2018-12-01T14:22:23Z

Contribution description

This is an implementation of the extension API as proposed in #9582 for PWM extensions. It corresponds to the implementation of the GPIO extension API in #9860 and #9958. The PR contains the following contributions:

Low level function prototypes pwm_*_ll in drivers/include/periph.h.
Changes of pwm_* functions to redirect a function call either to the low level functions pwm_*_ll or to the pwm_ext_* function provided by an PWM extension device driver.
Changes of low level function pwm_*_ll for all CPUs that support PWM devices.

The PWM extension API does not require feature periph_pwm which makes it possible to extend boards that do not provide PWM capabilities by the MCU by external PWM modules.

Testing procedure

A test case is provided that implements a soft-driver for the PWM extension interface. The test case uses this driver to confirm that interception and redirection of the API call are working properly. This has been tested and is working properly on esp8266-esp-12x, esp32-wroom-32 and arduino-mega2560.

To test only the PWM extension API use

make  flash test -C tests/extend_pwm BOARD=...

To test the PWM extension API together with internal PWM channels use

USEMODULE=periph_pwm make  flash test -C tests/extend_pwm BOARD=...

In that case feature periph_pwm is required of course.

Issues/PRs references

Implements #9582
Corresponds to #9860 and #9958

The changes in the makefiles/pseudomodules.mk and in drivers/Makefile.include made to get it working are the same as in #9958. These changes might be removed once #9958 is merged.

PR #10556 provides a working PWM extension driver that use the PWM extension API and serves as a proof of concept.

Adds low level function prototypes and redirection functions as well as some default macro definitions for PWM extensions to the API.

Renames all low level function for peripheral PWM drivers of MCUs

Adds the PWM extension driver including redirection and not supported functions

leandrolanzieri

Nice implementation. Just those comments. I ran the provided test in nucleo-f091rc and works as intended.

leandrolanzieri · 2018-12-12T15:50:38Z

drivers/extend/pwm_redir.c

+
+    if (entry == NULL) {
+        DEBUG("[%s] ext entry doesn't exist for dev %u\n", __func__, dev);
+        return -1;


On error 0 should be returned.

Suggested change

return -1;

return 0;

Yes, of course. Thanks.

leandrolanzieri · 2018-12-13T08:08:20Z

drivers/include/periph/pwm.h

+/**
+ * @brief   Default number of PWM extension devices
+ */
+#ifndef PWM_EXT_NUMOF


Should we only keep the definition of PWM_EXT_NUMOF if MODULE_EXTEND_PWM is not defined, instead of having it both here and in drivers/include/extend/pwm.h? Like:

#ifndef PWM_EXT_NUMOF #if !(MODULE_EXTEND_PWM) #define PWM_EXT_NUMOF (0U) #endif #endif

Ok, I just placed it also in drivers/include/periph/pwm.h to avoid wrong definitions if someone includes drivers/include/periph/pwm.h before drivers/include/extend/pwm.h.

This should be placed only in periph/pwm.h because it is included by extend/pwm.h, which means that periph/pwm.h is always loaded first.

Also, needs to be something like:

#ifndef PWM_EXT_NUMOF #if MODULE_EXTEND_PWM #define PWM_EXT_NUMOF (sizeof(pwm_ext_list) / sizeof(pwm_ext_list[0])) #else #define PWM_EXT_NUMOF (0U) #endif /* MODULE_EXTEND_PWM */ #endif /* PWM_EXT_NUMOF */

gschorcht · 2018-12-13T12:15:31Z

@leandrolanzieri Thank you for reviewing and testing.

ZetaR60

Looks good. atmega_common is missing the _ll functions though.

ZetaR60 · 2018-12-14T18:12:49Z

drivers/include/periph/pwm.h

+    }
+#else
+    return pwm_init_redir(dev, mode, freq, res);
+#endif


Please mark your #endifs with a comment after when you have nested #ifs: #endif /* MODULE_PERIPH_PWM */. This will make it much easier to read.

yes, of course.

ZetaR60 · 2018-12-14T18:15:47Z

tests/extend_pwm/Makefile

+include $(RIOTBASE)/Makefile.include
+
+test:
+	./tests/01-run.py


Explicitly referencing the test is no longer necessary. If ./tests/01-run.py exists, then it will be used.

ZetaR60 · 2018-12-14T18:44:50Z

Clarification on waiting for PR: This is waiting on approval of the methods outlined in #9582 via the approval by a senior maintainer of this or any of the other extension API PRs: #9860 + #9958, or #10512, #10527, #10533. They are all identical in method, and have only minor differences in implementation. As a split PR #9860 and #9958 are intended to make this easier.

gschorcht · 2018-12-29T10:41:15Z

@ZetaR60

atmega_common is missing the _ll functions though.

I don't know why atmega_common does not implement PWM peripherals. Each Arduino board has PWM channels.

stale · 2020-04-10T12:25:02Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

mguetschow · 2024-06-11T09:35:50Z

Since this PR has been stale for several years, I'll convert it to a draft.

Please feel free to remove the draft state if anyone wants to pick this up again.

This was referenced Dec 1, 2018

periph_*: Allow extensions for external periph adapters #9582

Closed

periph/gpio: enable API usage for expanders #9690

Open

gschorcht force-pushed the pwm_ext_api branch from 02e3ee5 to 45f6532 Compare December 1, 2018 14:40

gschorcht added 6 commits December 1, 2018 16:08

periph/pwm: initial PWM extension API

36cc3a5

Adds low level function prototypes and redirection functions as well as some default macro definitions for PWM extensions to the API.

cpu/*: support for PWM extension API

07adae7

Renames all low level function for peripheral PWM drivers of MCUs

drivers/extend: support for PWM extension API

2d6b64e

Adds the PWM extension driver including redirection and not supported functions

drivers: Makefile support for PWM extension API

ed1b48a

makefiles: support for PWM extension API

6aa55e9

test: PWM extension API test application

89229a1

gschorcht force-pushed the pwm_ext_api branch from 45f6532 to 89229a1 Compare December 1, 2018 15:08

gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers labels Dec 1, 2018

ZetaR60 self-assigned this Dec 6, 2018

gschorcht mentioned this pull request Dec 6, 2018

drivers: support for NXP PCA9685 I2C 16-channel, 12-bit PWM controller #10556

Merged

gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT and removed Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Dec 6, 2018

leandrolanzieri added Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Dec 12, 2018

leandrolanzieri requested changes Dec 13, 2018

View reviewed changes

gschorcht added 2 commits December 13, 2018 13:18

drivers/extend: fix of pwm_init_redir return value

439ee6f

periph/pwm: cleanup of PWM_EXT_NUMOF definition

66dc2f6

ZetaR60 suggested changes Dec 14, 2018

View reviewed changes

ZetaR60 mentioned this pull request Dec 14, 2018

periph/dac: support for DAC extension API #10512

Closed

ZetaR60 added this to the Release 2019.01 milestone Dec 14, 2018

gschorcht added 4 commits December 29, 2018 11:34

fixup! periph/pwm: initial PWM extension API

fa3310c

fixup! cpu/*: support for DAC extension API

9a4b51b

fixup! drivers/extend: support for DAC extension API

25db577

fixup! test: DAC extension API test application

6e2e832

fixup! periph/pwm: initial PWM extension API

6d73674

aabadie modified the milestones: Release 2019.01, Release 2019.04 Jan 25, 2019

miri64 modified the milestones: Release 2019.04, Release 2019.07 May 7, 2019

MrKevinWeiss modified the milestones: Release 2019.07, Release 2019.10 Jul 16, 2019

kb2ma removed this from the Release 2019.10 milestone Oct 8, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 10, 2020

gschorcht added State: don't stale State: Tell state-bot to ignore this issue and removed State: stale State: The issue / PR has no activity for >185 days labels Apr 10, 2020

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

mguetschow added State: stale State: The issue / PR has no activity for >185 days and removed State: don't stale State: Tell state-bot to ignore this issue labels Jun 11, 2024

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Jun 11, 2024

Conversation

gschorcht commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Dec 13, 2018

Uh oh!

ZetaR60 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZetaR60 commented Dec 14, 2018

Uh oh!

gschorcht commented Dec 29, 2018

Uh oh!

stale bot commented Apr 10, 2020

Uh oh!

mguetschow commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gschorcht commented Dec 1, 2018 •

edited

Loading