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

RFC: API: Counter: counter_read() has no way of indicating failure #21846

Closed
henrikbrixandersen opened this issue Jan 11, 2020 · 5 comments · Fixed by #22073
Closed

RFC: API: Counter: counter_read() has no way of indicating failure #21846

henrikbrixandersen opened this issue Jan 11, 2020 · 5 comments · Fixed by #22073
Assignees
Labels
area: API Changes to public APIs area: Counter RFC Request For Comments: want input from the community

Comments

@henrikbrixandersen
Copy link
Member

Introduction

The counter API has a method for reading the current counter value with the following function prototype:

u32_t counter_read(struct device *dev);

This API function does not allow any standard way of indicating a "read failure" since the return value is just the raw, 32-bit counter value.

Problem description

Counters, like most other devices, may be connected to the SoC/MCU using an external bus (I2C, SPI, ...) where communication errors can occur. Our current counter API allows indicating failures from all other API functions than the counter_read() function.

Proposed change

I suggest introducing a new counter API function for reading the counter value with the current function prototype:

int counter_get_value(struct device *dev, u32_t *ticks);

This new API function will effectively replace counter_read(), which can then be deprecated. During the deprecation period, counter_read() can be implemented using the newly introduced counter_get_value() API function and return 0 if counter_get_value() returned a negative error code.

Detailed RFC

  • Introduce counter_get_value() counter API syscall and handler
  • Deprecate counter_read() counter API syscall and handler and reimplement it using counter_get_value()
  • Introduce counter_api_get_value() typedef to replace existing counter_api_get_value()
  • Update all in-tree counter drivers to match the new counter driver API (most will just return a value of zero)

Proposed change (Detailed)

See above.

Dependencies

This change will require users updating any out-of-tree counter drivers, but the application API can remain the same during the deprecation period.

Concerns and Unresolved Questions

None so far.

Alternatives

None so far.

@henrikbrixandersen henrikbrixandersen added area: Counter RFC Request For Comments: want input from the community area: API Changes to public APIs labels Jan 11, 2020
@nordic-krch
Copy link
Contributor

Sounds ok. Counter API was initially created for internal peripherals thus direct readout but as it fits external devices then it should be extended.

@henrikbrixandersen
Copy link
Member Author

Great. Thanks for your feedback. What is the next step here? Should I create a pull request?

@carlescufi
Copy link
Member

carlescufi commented Jan 14, 2020

Great. Thanks for your feedback. What is the next step here? Should I create a pull request?

Yes, and you can also deprecate counter_read() in the same PR. Since this is not a Stable API change (without deprecation) you don't need to do anything extra in this case.

EDIT: @henrikbrixandersen I was wrong, this is listed as an unstable API so the only thing you need to do is to change the API call directly (no need to rename it, just change the signature) and then add a small note in the release notes.

@henrikbrixandersen
Copy link
Member Author

PR creation is awaiting #22030.

@andrewboie
Copy link
Contributor

+1 to this proposal

henrikbrixandersen added a commit to henrikbrixandersen/zephyr that referenced this issue Jan 21, 2020
Introduce a new counter API function for reading the current counter
value (counter_get_value()) and deprecate the former counter_read() in
favor of this.

Update all drivers and calling code to match the new counter API.

The previous counter driver API function for reading the current value
of the counter (counter_read()) did not support indicating whether the
read suceeded. This is fine for counters internal to the SoC where the
read always succeeds but insufficient for external counters (e.g. I2C
or SPI slaves).

Fixes zephyrproject-rtos#21846.

Signed-off-by: Henrik Brix Andersen <[email protected]>
nashif pushed a commit that referenced this issue Jan 28, 2020
Introduce a new counter API function for reading the current counter
value (counter_get_value()) and deprecate the former counter_read() in
favor of this.

Update all drivers and calling code to match the new counter API.

The previous counter driver API function for reading the current value
of the counter (counter_read()) did not support indicating whether the
read suceeded. This is fine for counters internal to the SoC where the
read always succeeds but insufficient for external counters (e.g. I2C
or SPI slaves).

Fixes #21846.

Signed-off-by: Henrik Brix Andersen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Counter RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants