Skip to content

Conversation

@eldruin
Copy link
Contributor

@eldruin eldruin commented Aug 2, 2024

I have noticed that the addresses supplied in the transaction start/end are not checked. Hence this PR.
Although I am wondering now if the transaction start/end should have an address parameter at all. Maybe we should remove it altogether.

@dbrgn
Copy link
Owner

dbrgn commented Aug 2, 2024

Although I am wondering now if the transaction start/end should have an address parameter at all. Maybe we should remove it altogether.

How would you pass the address argument to the read/write functions in that case?

@eldruin
Copy link
Contributor Author

eldruin commented Aug 5, 2024

The address is already checked in the individual steps.

[ I2cTrans::transaction_start(ADDR),
  I2cTrans::write(ADDR, vec![0]),
  I2cTrans::read(ADDR, &mut data),
  I2cTrans::transaction_end(ADDR) ]

vs.

[ I2cTrans::transaction_start(),
  I2cTrans::write(ADDR, vec![0]),
  I2cTrans::read(ADDR, &mut data),
  I2cTrans::transaction_end() ]

@asasine
Copy link
Contributor

asasine commented Aug 5, 2024

Removing the address from the transaction start is opposite to how the underlying bus works for multiple operations. The address is sent as part of start/repeated start, and not as part of the write/read operations. If anything, the address should be only included in the starts and removed from everything else.

@2bndy5
Copy link

2bndy5 commented Apr 25, 2025

According to actual I2C specifications, the address shall be altered

let is_rw = match e.expected_mode {
    Mode::Write => 0u8,
    Mode::Read => 1u8,
};
let addr = (e.expected_address << 1) | is_rw;

But technically, this would be done in the hal implementation, so this is superfluous in mock tests.

I say this because an I2C address cannot exceed 7 bits. This is something that mocks should be checking.

I do agree with @asasine though. Repeatedly specifying the address for parts of the same transaction in I2C expectations does not make sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants