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: i2c: document "slave" API #27996

Merged
merged 6 commits into from
Feb 9, 2021

Conversation

pabigot
Copy link
Collaborator

@pabigot pabigot commented Sep 2, 2020

Document the API used for I2C slave devices. Fill out the peripheral documentation with a reference to the I2C specification, a short summary of the roles of I2C devices, and a note on language used. Remove some stubs from drivers that don't implement the slave API. The focus of the PR is documenting the existing following API, not documenting the I2C subsystem as a whole.

The PR includes a change that distinguishes the slave API as Experimental in the API stability table, as it is not satisfactory and has limited support. See also #27675.

The documented behavior is derived primarily from the LPC11U6X implementation, as the STM32 implementation generally ignores return values. There may be errors in the description, and there are certainly gaps in the existing driver implementations.

Updated 2021-02-06 to revert to "master/slave" terminology in conformance to the coding guidelines.

Fixes #27879
References #27033

@olofj
Copy link
Contributor

olofj commented Sep 3, 2020

I don't think the "leader" and "follower" terminology makes sense in the i2c context -- the i2c target/device isn't "following" the leader in the same way that a main/replica database would do, for example.

"i2c controller" or "i2c host" or "i2c initiator" and "i2c device" or "i2c target" seem like more appropriate terminology here, unless leader/follower has already gained significant use elsewhere. I think this is also consistent with some of the feedback on #27033.

franciscomunoz
franciscomunoz previously approved these changes Sep 3, 2020
Copy link
Contributor

@franciscomunoz franciscomunoz left a comment

Choose a reason for hiding this comment

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

Overall looks good. I don't have anything to comment about the terms master/slave.

@pabigot
Copy link
Collaborator Author

pabigot commented Sep 3, 2020

@olofj I've added #27033 (comment) to further explain why I'm using these terms. Please provide further feedback on the name selection in that PR.

I2C
####

Overview
********

.. note::

Zephyr recognizes the need to change the racially-charged terms
Copy link
Member

Choose a reason for hiding this comment

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

IMO this type of note should be done in one place in the Zephyr documentation and not per peripheral.

@nashif
Copy link
Member

nashif commented Sep 3, 2020

@olofj I've added #27033 (comment) to further explain why I'm using these terms. Please provide further feedback on the name selection in that PR.

We should probably consider splitting this PR and not mix a documentation bug fix with inclusive language changes that still being discussed.

@pabigot
Copy link
Collaborator Author

pabigot commented Sep 3, 2020

The documentation requires use of three distinguishing terms, currently "controller", "leader", and "follower". We can remove the explanation of why those terms are used if necessary.

I would not be happy about being forced to change the documentation to continue to use "master" and "slave".

@nashif
Copy link
Member

nashif commented Sep 4, 2020

I would not be happy about being forced to change the documentation to continue to use "master" and "slave".

making this change here and leaving i2c with slave/master usage especially in with the APIs still using slave/master in their function names is not going to make anyone happy. Once we know and agree on what terms we should be using this should be done across the tree, not only for the "slave".

@pabigot
Copy link
Collaborator Author

pabigot commented Sep 7, 2020

At this time no changes have been requested, just comments that express opinions about the use of "leader" and "follower". My opinion differs, or I'd have done something else. My detailed responses are at/around #27033 (comment).

Here I would still like reviews related to the technical content, so we can move forward on documenting this API.

@nashif
Copy link
Member

nashif commented Oct 21, 2020

Can we move this forward based on the discussion we had 2 weeks ago re inclusive language please?

scottwcpg
scottwcpg previously approved these changes Oct 21, 2020
@pabigot
Copy link
Collaborator Author

pabigot commented Oct 21, 2020

Waiting on this action item for the decision on how Zephyr will be addressing non-inclusive language.

Transitioning Subsystems
Create page on documentation for transition, including table with status of subsystems
Skeleton for the page - Maureen
Subsystem maintainers own updating status / plans

carlescufi
carlescufi previously approved these changes Oct 30, 2020
@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label Nov 11, 2020
@nashif
Copy link
Member

nashif commented Dec 2, 2020

Waiting on this action item for the decision on how Zephyr will be addressing non-inclusive language.

@MaureenHelm any updates?

@MaureenHelm
Copy link
Member

Waiting on this action item for the decision on how Zephyr will be addressing non-inclusive language.

@MaureenHelm any updates?

Working on a PR now.

@zephyrbot zephyrbot added the platform: Microchip MEC Microchip MEC Platform label Jan 8, 2021
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Feb 6, 2021
@pabigot pabigot removed platform: Microchip MEC Microchip MEC Platform DNM This PR should not be merged (Do Not Merge) TSC Topics that need TSC discussion labels Feb 6, 2021
@pabigot pabigot dismissed stale reviews from scottwcpg, franciscomunoz, and carlescufi February 6, 2021 13:26

content changed to conform to current Zephyr coding guidelines

@pabigot pabigot changed the title drivers: i2c: document follower ("slave") API drivers: i2c: document "slave" API Feb 6, 2021
Documentation for subsystems that use offensive terms by standard may
need to reference this section as justification for their continued
and new use.

Signed-off-by: Peter Bigot <[email protected]>
Status of addressing inclusive language concerns is embedded into the
coding guidelines.  Add a link to the corresponding issue for I2C.

Signed-off-by: Peter Bigot <[email protected]>
Zephyr does not currently allow deviation from standard terminology
for a technology even if it is non-inclusive, until the corresponding
standards body has confirmed intent to change that terminology.  The
terms used in a previous attempt to be inclusive do not match the
expected forthcoming standard terms.

Revert to standard terms until the new ones have been announced and
the switch made throughout Zephyr.

Signed-off-by: Peter Bigot <[email protected]>
The callback pointers for uninitiated operations are implicitly null;
making them explicit only confuses maintainers searching for drivers
that implement the API.

Signed-off-by: Peter Bigot <[email protected]>
The I2C slave API has been present since 1.12 but lacked documentation
on the behavior of its functions.  Provide that, and revise existing
documentation to make clear the type and mode of the device passed to
each API call.

Documented behavior is derived primarily from the LPC11U6X
implementation, as the STM32 implementation generally ignores return
values.

Signed-off-by: Peter Bigot <[email protected]>
Although the master API is stable, the slave API has never been
documented and has only two in-tree implementations with no uses
outside of a single test application.  It is marked experimental in
this commit.

Signed-off-by: Peter Bigot <[email protected]>
@nashif nashif added this to the v2.5.0 milestone Feb 6, 2021
@pabigot pabigot removed this from the v2.5.0 milestone Feb 9, 2021
@nashif
Copy link
Member

nashif commented Feb 9, 2021

isnt this a documentation change? Why should this not go into 2.5?

@pabigot
Copy link
Collaborator Author

pabigot commented Feb 9, 2021

If you want it there, sure. I was cleaning up the PRs I'd wanted to get in to this version, and this wasn't specifically one of them because of the delay in getting the policy in place. Feel free to add it back if you intend to merge it.

@pabigot pabigot added this to the v2.5.0 milestone Feb 9, 2021
@pabigot
Copy link
Collaborator Author

pabigot commented Feb 9, 2021

Ah, my mistake, I thought this was a milestone I'd assigned. Back now.

@nashif nashif merged commit a31b3c4 into zephyrproject-rtos:master Feb 9, 2021
@pabigot pabigot deleted the nordic/20200902d branch February 22, 2021 13:29
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: Documentation area: I2C area: Tests Issues related to a particular existing or missing test Inclusive Language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make i2c_slave callbacks public in the documentation
8 participants