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/dac: support for DAC extension API #10512

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Nov 29, 2018

Contribution description

This is an implementation of the extension API as proposed in #9582 for DAC 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 dac_*_ll in drivers/include/periph.h.
  • Changes of dac_* functions to redirect a function call either to the low level functions dac_*_ll or to the dac_ext_* function provided by an DAC extension device driver.
  • New function prototype dac_channels which returns the number of channels, in case of low level drivers simply the DAC_NUMOF. This new function makes it possible to get the information from a certain device how many channels it has and allows to iterate over the all channels of all devices.
  • Changes of low level function dac_*_ll for all CPUs that support DAC channels.

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

Testing procedure

A test case is provided that implements a soft-driver for the DAC 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 DAC extension API use

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

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

USEMODULE=periph_dac make  flash test -C tests/extend_dac BOARD=...

In that case feature periph_dac 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 #10518 provides a working DAC extension driver that use the DAC extension API and serves as a proof of concept.

@gschorcht gschorcht force-pushed the dac_ext_api branch 2 times, most recently from 38cba7f to c74c795 Compare November 30, 2018 15:24
@miri64 miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Nov 30, 2018
@gschorcht gschorcht changed the title periph/dac: support for extension API periph/dac: support for DAC extension API Dec 1, 2018
Adds low level function prototypes and redirection functions to the API. Furthermore, a new API function dac_channels was added to make it possible to iterate over all channels of a certain device.
Renames all low level function for peripheral DAC drivers of MCUs
Adds the DAC extension driver including redirection and not supported functions
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.

Everything looks good, except for a few things involving the bits of the API that haven't been completely settled yet.

I think once #9860 and #9958 are merged, this will be easy to merge due to similarity, even though this is a major change.

#ifndef DAC_EXT_LINE
#define DAC_EXT_LINE(x, y) \
(dac_t)(~DAC_EXT_THRESH | (x << DAC_EXT_DEV_LOC) | DAC_LINE(y))
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

May need to be changed, but required changes are to be determined. Discussion in #10504

* @param[in] line identifies the device
* @retval the number of channels on success or 0 on error
*/
static inline unsigned int dac_channels(dac_t line)
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be relevant to how lines are enumerated, as discussed in #9582 and #10504. This might become the standard method, or it might need to be changed based on those discussions.

drivers/Makefile.dep Show resolved Hide resolved
include ./Makefile.include
include $(RIOTBASE)/Makefile.include

test:
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 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. 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 10, 2018
@ZetaR60 ZetaR60 added this to the Release 2019.01 milestone Dec 10, 2018
@gschorcht
Copy link
Contributor Author

@ZetaR60 Thank you for reviewing

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

@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
@miri64
Copy link
Member

miri64 commented Jun 24, 2020

Is @ZetaR60 still active? Last activities according to their landing page was in Feb 2019. Should someone else take over the review?

@gschorcht
Copy link
Contributor Author

This PR was part of a bundle of PRs that extend the peripheral APIs for extender drivers (#9582). For that purpose, it uses the same redirection approach for DACs as PRs #9860 and #9958 did for GPIOs.

Since the redirection approach for GPIOs in PR #9860 and #9958 was finally refused, a new approach for GPIO extension API was developed and discussed in PR #12877. Once the GPIO extension API is finished and accepted, I will rework the other extension APIs. Another approach that could also be suitable for DACs is the ADC-NG API in PR #13248.

So at the moment I see this PR more as a reminder than a PR that should be merged.

@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
@Teufelchen1 Teufelchen1 marked this pull request as draft March 18, 2024 13:22
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.

6 participants