Skip to content

drivers/periph/spi: clean up error codes and doc#15896

Merged
jeandudey merged 1 commit intoRIOT-OS:masterfrom
maribu:spi-api-cleanup
Feb 27, 2021
Merged

drivers/periph/spi: clean up error codes and doc#15896
jeandudey merged 1 commit intoRIOT-OS:masterfrom
maribu:spi-api-cleanup

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Feb 1, 2021

Contribution description

  • Use negative errno as error codes, rather than home-grown enums
    • Update the home-grown enum with negative errno codes for backward compatibility and mark it deprecated
    • This brings the API back to current coding practices in RIOT, e.g. as documented in the driver guide
  • Update API doc to use negative errno codes
  • Fix various style issues in Doxygen doc
    • Use @retval to document specific return values instead of abusing @return for this
    • Align parameters to proper indent level

Note on API Change

As this change is fully backward compatible, there should not be any issues. But marking previous behavior (using e.g. SPI_OK) as deprecated IMO qualifies this as an API change.

Testing procedure

  • Generated binaries should be almost identical. (Using different constants in return values will change binaries, obviously)
  • All SPI tests should still work

Issues/PRs references

None

@maribu
drivers/periph/spi: clean up error codes and doc
ae79f5f 
- Use negative errno as error codes, rather than home-grown enums
    - Update the home-grown enum with negative errno codes for backward
      compatibility and mark it deprecated
- Update API doc to use negative errno codes
- Fix various style issues in Doxygen doc
    - Use `@retval` to document specific return values instead of abusing
      `@return` for this
    - Align parameters to proper indent level
@maribu maribu added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Feb 1, 2021
@maribu maribu requested review from aabadie and benpicco February 1, 2021 10:09
@maribu maribu requested a review from MrKevinWeiss as a code owner February 1, 2021 10:09
@maribu maribu mentioned this pull request Feb 1, 2021
Merged
2 tasks
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 2, 2021
benpicco
benpicco approved these changes Feb 2, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Nobody should depend on the exact numeric values of the enum anyway, as long as 0 stays success and errors remain negative.

@benpicco benpicco requested a review from fjmolinas February 2, 2021 11:53
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 2, 2021 •
edited
Loading

The only failed test is an CI hiccup and completely unrelated to this PR, so I trigger a re-run without compilation.

Restarting CI with compile test re-enabled. The issues has been fixed and this time it should work.

@maribu maribu added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs 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 Feb 2, 2021
@benpicco benpicco requested a review from kaspar030 February 2, 2021 14:47
@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 CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Feb 2, 2021
jeandudey
jeandudey approved these changes Feb 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jeandudey jeandudey left a comment

Choose a reason for hiding this comment

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

LGTM, Documentation matches the changes. It's very unlikely that someone's else code is depending on magic numbers rather than the provided enum values.

@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 Feb 26, 2021
@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 26, 2021

FYI tests/congure_test is failing since a docker container is outdated. This should be fixed soon (alternatively try restarting the build until you don't hit that one for native tests...).

@jeandudey jeandudey 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 Feb 26, 2021
@jeandudey jeandudey merged commit a30184b into RIOT-OS:master Feb 27, 2021
@jeandudey
Copy link
Copy Markdown
Contributor

jeandudey commented Feb 27, 2021

Officially I've pressed the green button :P 🎉

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Feb 27, 2021

Hooray!

@maribu maribu deleted the spi-api-cleanup branch February 27, 2021 07:00
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@jeandudey jeandudey jeandudey approved these changes

@aabadie aabadie Awaiting requested review from aabadie

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

No one assigned

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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2021.04

Development

Successfully merging this pull request may close these issues.

5 participants

@maribu @miri64 @jeandudey @benpicco @kaspar030