Skip to content

Wire fixes for repeated start(slave) and multiple .write in onRequestEvent() #1221

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

Merged
merged 2 commits into from
Oct 30, 2020

Conversation

DanielLiebler
Copy link
Contributor

Summary

This PR fixes/implements the following bugs/features

  • when in slave mode and we are receiving a write/read combo with a repeated start(so no stop condition in between), the write part doesn't trigger an onReceiveEvent()
  • when doing multiple .write() calls in onRequestEvent(), only the last one is used. (as outlined in could'nt recieve integers at master from slave #488 )

Description

  • The slave implementation doesn't seem to work with repeated starts, but this is also a quick fix by sending out an onReceiveEvent() when getting the new address callback.

  • In could'nt recieve integers at master from slave #488 it was mentioned to just use one write, but I think this is not good behavior, since it's not mentioned in the documentation. Nevertheless, it seems to be a quick fix, so I just created this PR.

How it works

  • When getting the address callback, the data which has been cached is sent to an onReceiveEvent, when still in SLAVE_MODE_RECEIVE mode.

  • We are resetting i2cTxRxBufferSize before calling i2c_onSlaveTransmit() in HAL_I2C_AddrCallback()
    and modify the function i2c_slave_write_IT() to start writing in the i2cTxRxBuffer with the size of the buffer as offset and add up the sizes.

@fpistm fpistm requested a review from ABOSTM October 29, 2020 09:41
Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

Hi @DanielLiebler,
Thanks for your Pull Request.
After review, it looks good to me,
but I would like to test, specially because there are 2 major I2C Hardware implementation within STM32 families.
Can you tell me what you tested ? which boards ...
Can you tell me how you tested ? Which master with repeated start?
Can you share you sketch () ?

@ABOSTM
Copy link
Contributor

ABOSTM commented Oct 29, 2020

Thank I will test that.

@fpistm
Copy link
Member

fpistm commented Oct 29, 2020

Thanks @DanielLiebler for this PR.
Could you squash your last commit with the first one please.

Note: The CI core compilation issue is normal as I'm updating some dependencies and will be back to normal soon.

@DanielLiebler
Copy link
Contributor Author

@fpistm should be done. I didnt know you could do such things, even though I guess you can mess up things really bad with squashing^^

@fpistm fpistm added enhancement New feature or request fix 🩹 Bug fix labels Oct 29, 2020
@fpistm fpistm added this to the 2.0.0 milestone Oct 29, 2020
@fpistm
Copy link
Member

fpistm commented Oct 29, 2020

Thanks @DanielLiebler
I've finished to update dependencies anyway to have the build OK, could you rebase on top of the master. Thanks in advance.

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

LGTM
Tested on Nucleo F429ZI (I2C V1) and Nucleo F746ZG (I2C V2)

Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

LGTM
Waiting @ABOSTM approval

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

LGTM

@fpistm fpistm merged commit 554a0a8 into stm32duino:master Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix 🩹 Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants