Skip to content
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

periph/pwm: support for PWM extension API #10533

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Dec 1, 2018

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
@gschorcht 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 ZetaR60 self-assigned this Dec 6, 2018
@gschorcht 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 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
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On error 0 should be returned.

Suggested change
return -1;
return 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. Thanks.

/**
* @brief Default number of PWM extension devices
*/
#ifndef PWM_EXT_NUMOF
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@leandrolanzieri Thank you for reviewing and testing.

Copy link
Contributor

@ZetaR60 ZetaR60 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. atmega_common is missing the _ll functions though.

}
#else
return pwm_init_redir(dev, mode, freq, res);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, of course.

include $(RIOTBASE)/Makefile.include

test:
./tests/01-run.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ZetaR60 ZetaR60 added State: waiting for other PR State: The PR requires another PR to be merged first Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Dec 14, 2018
@ZetaR60 ZetaR60 added this to the Release 2019.01 milestone Dec 14, 2018
@ZetaR60
Copy link
Contributor

ZetaR60 commented Dec 14, 2018

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
Copy link
Contributor Author

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

@kb2ma kb2ma removed this from the Release 2019.10 milestone Oct 8, 2019
@stale
Copy link

stale bot commented Apr 10, 2020

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.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 10, 2020
@gschorcht 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 MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers 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. Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines State: don't stale State: Tell state-bot to ignore this issue State: waiting for other PR State: The PR requires another PR to be merged first Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants