Skip to content

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

Merged
benpicco merged 1 commit into
RIOT-OS:masterfrom
fabian18:drivers/include/periph/eeprom-api-change
Aug 11, 2019
Merged

drivers/include/periph/eeprom: Changed uint8_t* to void* in API#11990
benpicco merged 1 commit into
RIOT-OS:masterfrom
fabian18:drivers/include/periph/eeprom-api-change

Conversation

@fabian18

@fabian18 fabian18 commented Aug 9, 2019

Copy link
Copy Markdown
Contributor

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

maribu commented Aug 9, 2019

Copy link
Copy Markdown
Member

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

maribu commented Aug 9, 2019

Copy link
Copy Markdown
Member

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

@maribu maribu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK (1/2)

@benpicco benpicco left a comment

Copy link
Copy Markdown
Contributor

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