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

drivers/include/periph/eeprom: Changed uint8_t* to void* in API #11990

Merged

Conversation

fabian18
Copy link
Contributor

@fabian18 fabian18 commented Aug 9, 2019

Contribution description

This tiny PR changes the EEPROM API for eeprom_read and eeprom_write
to expect void* data instead of uint8_t* data. This suggests a more general interface.

Testing procedure

I executed tests/eepreg and tests/periph_eeprom on arduino-mega2560.

Issues/PRs references

This PR has emerged from PR #11929 .

@maribu maribu added 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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers labels Aug 9, 2019
@maribu
Copy link
Member

maribu commented Aug 9, 2019

tests/periph_eeprom:

> write 0 Hallo
2019-08-09 18:12:46,228 - INFO #  write 0 Hallo
2019-08-09 18:12:46,269 - INFO # 5 bytes written to EEPROM
> read 0 5
2019-08-09 18:12:52,287 - INFO #  read 0 5
2019-08-09 18:12:52,327 - INFO # Data read from EEPROM (5 bytes): Hallo

tests/eepreg:

2019-08-09 18:13:50,484 - INFO # main(): This is RIOT! (Version: 2019.10-devel-365-g2c001-fabian)
2019-08-09 18:13:50,491 - INFO # EEPROM registry (eepreg) test routine
2019-08-09 18:13:50,573 - INFO # Testing new registry creation: reset check [SUCCESS]
2019-08-09 18:13:50,684 - INFO # Testing writing and reading entries: add write add read [SUCCESS]
2019-08-09 18:13:50,740 - INFO # Testing detection of conflicting size: add [SUCCESS]
2019-08-09 18:13:50,790 - INFO # Testing calculation of lengths: len len [SUCCESS]
2019-08-09 18:13:51,073 - INFO # Testing of successful data move after rm: rm read data [SUCCESS]
2019-08-09 18:13:51,162 - INFO # Testing of free space change after write: free add free [SUCCESS]
2019-08-09 18:13:51,224 - INFO # Testing of iteration over registry: iter bar foo [SUCCESS]
2019-08-09 18:13:51,240 - INFO # Tests complete!

@maribu maribu added Reviewed: 3-testing The PR was tested according to the maintainer guidelines CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Aug 9, 2019
@maribu
Copy link
Member

maribu commented Aug 9, 2019

As this is an API change, 2 ACKs are formally required. (This change will not break any caller, as uint8_t * can be implicitly casted to void *.)

@benpicco: Mind to have a look?

@maribu maribu added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Aug 9, 2019
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK (1/2)

Copy link
Contributor

@benpicco benpicco 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. Interestingly both implementations already cast the argument to uint8_t* internally, so there really is no semantic change here.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 11, 2019
@benpicco benpicco merged commit 700b121 into RIOT-OS:master Aug 11, 2019
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
@fabian18 fabian18 deleted the drivers/include/periph/eeprom-api-change branch January 11, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK 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: 3-testing The PR was tested 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants