Skip to content

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Jun 6, 2018

Contribution description

This PR readapts the hts221 driver to the I2C return codes (< 0 in case of an error, 0 on success).

Issues/PRs references

Follow-up of #9195 and related to #6577

@aabadie aabadie added Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force labels Jun 6, 2018
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

Overall changes are OK,
Could you share some debug output or do you know if someone can test this device so we can move forward ?
BTW, why acquire/release are used several times in the init function ? never saw this behaviour.

@aabadie
Copy link
Contributor Author

aabadie commented Jun 21, 2018

why acquire/release are used several times in the init function ?

I looked at the code more in depth, and this is perfectly fine in term of workflow: it's just that some private functions are not acquiring the bus before performing their read/write operations.

here is an output with a nucleo-l073:

2018-06-21 11:25:57,327 - INFO # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Type '/exit' to exit.
2018-06-21 11:25:58,801 - INFO # H: 56.4%, T: 24.8°C
2018-06-21 11:26:00,805 - INFO # H: 56.1%, T: 24.8°C
2018-06-21 11:26:02,803 - INFO # H: 56.0%, T: 24.8°C
2018-06-21 11:26:04,807 - INFO # H: 55.9%, T: 24.8°C

@dylad
Copy link
Member

dylad commented Jun 21, 2018

I looked at the code more in depth, and this is perfectly fine in term of workflow: it's just that some private functions are not acquiring the bus before performing their read/write operations.

Sure, but this is first time I saw this. Is there any gain from doing this ? I don't have strong opinion about this. If the driver was already in this state before, this is not our rights to change it with the I2C refactoring.
Anyway, it would be great if someone could test it, otherwise I can leave an untested ACK.

@aabadie
Copy link
Contributor Author

aabadie commented Jun 22, 2018

it would be great if someone could test it

I think @smlng has the hardware, since he initially wrote this driver. Maybe he can comment on your other questions.

@aabadie aabadie merged commit c20a6fe into RIOT-OS:new_i2c_if Jun 28, 2018
basilfx pushed a commit to basilfx/RIOT that referenced this pull request Jul 10, 2018
drivers/hts221: adapt to i2c api return codes
dylad pushed a commit to dylad/RIOT that referenced this pull request Jul 10, 2018
drivers/hts221: adapt to i2c api return codes
dylad pushed a commit that referenced this pull request Jul 11, 2018
drivers/hts221: adapt to i2c api return codes
@aabadie aabadie deleted the new_i2c_hts221 branch August 31, 2018 07:05
@aabadie aabadie added this to the Release 2018.10 milestone Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers TF: I2C Marks issues and PRs related to the work of the I²C rework task force
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants