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

add i2c trait #64

Merged
merged 4 commits into from
Mar 6, 2021
Merged

add i2c trait #64

merged 4 commits into from
Mar 6, 2021

Conversation

xoviat
Copy link
Contributor

@xoviat xoviat commented Mar 2, 2021

closes #51.

@xoviat
Copy link
Contributor Author

xoviat commented Mar 2, 2021

mostly copied from embedded-hal; transactional mod can be sorted later

embassy-traits/src/i2c.rs Outdated Show resolved Hide resolved
embassy-traits/src/i2c.rs Outdated Show resolved Hide resolved
embassy-traits/src/i2c.rs Outdated Show resolved Hide resolved
@Dirbaio
Copy link
Member

Dirbaio commented Mar 6, 2021

I'm not sure about the design. There are many traits: Read, Write, WriteIter, WriteRead, WriteIterRead, Transactional.

  • What's the advantage in separating Read, Write, WriteRead? I don't think there's hardware that supports only reading but not writing or viceversa.

  • WriteIter, WriteIterRead don't seem very useful to me if we're going to implement this with DMA. It forces impls to write byte-by-byte.

The Transactional one is nice to have :D Unfortunately it's not possible to implement it on nrf52, but I imagine it can be useful for other chips/

@eupn
Copy link

eupn commented Mar 6, 2021

@Dirbaio as far as Read trait concerned, there are chips (BNO08x for example) that use I2C only as a transport (like uart, etc.) for bytes and doesn’t rely on first writing a register address before reading data.

@Dirbaio
Copy link
Member

Dirbaio commented Mar 6, 2021

Yeah, having read, write, write_read methods make sense. I was wondering why is it necessary to have 3 traits, instead of 1 trait with 3 methods.

Also, read and write could really be just write_read with zero-size slices, so having just a write_read method could work too

@xoviat
Copy link
Contributor Author

xoviat commented Mar 6, 2021

This is copied from embedded-hal. Why are there different traits there?

@Dirbaio
Copy link
Member

Dirbaio commented Mar 6, 2021

I did some archeology on embedded-hal, only thing I could find is this. The commit that adds them already has them this way.

Also from Matrix:

Dirbaio: Does anyone know why embedded-hal traits are always 1 method per trait?
Dirbaio: For example: for i2c theres Read, Write, WriteRead traits with one method each. 
jamesmunns: Generally more flexibility
Dirbaio: what's the advantage vs having 1 trait with the 3 methods? 
Dirbaio: I kinda see it with uart read/write, you may want to "split" it
Dirbaio: but that doesn't make sense at all with i2c
jamesmunns: If you have hardware that can only do 2 of those three things, it can work with drivers that don't need that thing?
Dirbaio: is there hardware that can only i2c-write and can't i2c read? 
Dirbaio: or viceversa?
jamesmunns: I mean
jamesmunns: Not i2c, but spi yeah
jamesmunns: Like a tx only spi port for ws2812b spi
Dirbaio: hm, right, for uart/spi it does make sense
jamesmunns: Then probably just consistency

It's a good point for UART and SPI. The current UART trait isn't split, we should maybe split it. Still, i'm not very convinced it's good to split I2C (neither for the errors nor for consistency)

@Dirbaio
Copy link
Member

Dirbaio commented Mar 6, 2021

Don't delete write_read. It's not the same as write + read: It does a Repeated Start, instead of Stop+Start.

@Dirbaio
Copy link
Member

Dirbaio commented Mar 6, 2021

👍

@Dirbaio Dirbaio merged commit 3b95e1a into embassy-rs:master Mar 6, 2021
Dirbaio added a commit that referenced this pull request May 30, 2023
general maintenance commits
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.

I2C traits
3 participants