search

RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.9k stars 1.98k forks source link

periph/i2c: tracking the remodeling #6577

Closed haukepetersen closed 6 years ago

haukepetersen commented 7 years ago

The changes proposed in #6575 and #6576 need many efforts to be undertaken. This issue will track the progress and should help to synchronize the work efforts.

Modules in RIOT that need adaption: CPUS:

DRIVERS AND TESTS:

TESTS:

PACKAGES:

dylad commented 6 years ago

just one remark, adxl345 driver is missing from the list.

ZetaR60 commented 6 years ago

@haukepetersen ads101x update: #9165

vincent-d commented 6 years ago

For stm32, we should try to come up with a unified driver. It seems that there are 2 different IPs:

So 2 versions of the driver should be written.

aabadie commented 6 years ago

@vincent-d, +1 for unifying the drivers but it seems simpler/harmless to do it in a second step. Is it ok for you ?

vincent-d commented 6 years ago

I would say it's easier to do it straight. But of course, I would develop with only 1 arch first and adapt afterwards. But the current situation with a driver per sub-family where most of them are just copy/paste is crap.

aabadie commented 6 years ago

Ok @vincent-d, you convinced me. See #9202 for the L0 and F3. I don't know how to cleanly set the internal speed options and how to unify with the second type of drivers (F1/F2/F4/L1). Are you ok to work on this part ?

vincent-d commented 6 years ago

So L0/F3 should work on F0/F7/L4 as well. And as it seems quite different for the others, the best would maybe to start with two different versions. I can help, but I'll be quite busy next week, so not sure if I'll have time :/ Thanks for starting that btw!

jnohlgard commented 6 years ago

I will try to do the Kinetis implementation during the weekend

dylad commented 6 years ago

Great !! Many thanks @gebart !:+1:

MrKevinWeiss commented 6 years ago

Just FYI,

I am testing with the new i2c API and a samr21 board,

the new API says the return of i2c read/writes should be error codes ( I2C_ACK, I2C_ADDR_NACK...) but they are return the number of bytes.

Also the read bytes and read reg seem to be physically reading one too many: i2c_read_bytes(addr, 4, 0) actually reads 5 bytes (according to the oscilloscope) but says it reads 4.

dylad commented 6 years ago

the new API says the return of i2c read/writes should be error codes ( I2C_ACK, I2C_ADDR_NACK...) but they are return the number of bytes.

In fact, when we implemented the proposed I2C API, we also look at the current implementation of SPI. I guess the number of bytes read or written is borrowed from the SPI and we didn't take in account the I2C_ACK return code. Both solutions can be applied we just need a consensus I guess :)

Regarding the extra byte read, as I remember this issue is also true in the previous API. We need to ensure this issue only exists for SAM0 CPUs. I'll try to fix whenever I can but it can also be fix later if it's only impacting SAM0.

jnohlgard commented 6 years ago

What about slave mode? This would be a good time to at least think about where slave mode will fit in the API. Since i2c is a multimaster bus, the API should be designed in a way that a device can act both as a master and a slave on the same bus.

dylad commented 6 years ago

What about slave mode?

TBH, I don't really think about it but you're right, this may be a good timing to start some discussions about like how the slave API will look like ? Regarding multimaster bus, I've personally never used it. Do we know if there are some existing, working and open-source implementations somewhere ? This may fit in another issue.

jnohlgard commented 6 years ago

One more open issue: How do we configure timeout limits for when the slave holds the clock low for too long?

dylad commented 6 years ago

What about create a device attribute as params ? Or maybe add a hardcoded value per driver ?

aabadie commented 6 years ago

@dylad, #9202 (STM32 I2C driver) has rebase issue with the new_i2c_if branch because #7658 (DMA) was merged after the embargo in master (Travis is trying to rebase against master) and it conflicts with #9202 since they touch the same files. I'm wondering what it the best strategy to solve this. Should we rebase new_i2c_if branch on top of master now ? or backport #7658 to new_i2c_if ? or something else ?

dylad commented 6 years ago

This is where the fun begins. I guess we should rebase the new_i2c_if branch. It's probably better than push a revert which will re-revert right ?

aabadie commented 6 years ago

we should rebase the new_i2c_if branch

I would do that as well, because this is what will happen in the end. Maybe @miri64 will have some advice, since she already managed the netif feature branch.

If we do the rebase, the next question is: do we want to keep the merge commit in the rebased branch (using --preserve-merges) or not ?

dylad commented 6 years ago

do we want to keep the merge commit in the rebased branch (using --preserve-merges) or not ?

I'm afraid I don't understand a single word of your question... I'm not a git expert.

aabadie commented 6 years ago

I'm not a git expert.

No problem. The current RIOT workflow in GitHub creates merge commits each time one hits the GitHub green button. So for example the new_i2c_if branch contains all merge commits created when we merged PRs containing updates of I2C API and drivers (have a look at the git history, merge commits have 2 ancestors commits - for other git experts, let's keep it simple for now). But when one rebases a branch with merge commits (here new_i2c_if) on top of another branch (here it would be master), the default behaviour of git is to remove them. Instead, you can force git to try to also reapply the merge commits using the --preserve-merges option. Personally, I would vote for removing the merge commits (I don't like it, I prefer a linear history).

dylad commented 6 years ago

Well I guess I will leave this question to our zealots :) TBH I understand the situation but I don't have strong opinion.

miri64 commented 6 years ago

Maybe @miri64 will have some advice, since she already managed the netif feature branch.

If you don't want to loose merge commits (and there identity) the way to go would be to merge master into the branch, but this should have been prevented by the embargo. So the cleanest way would be to revert or backport the commits that cause the conflict/are required.

dylad commented 6 years ago

So the cleanest way would be to revert or backport the commits that cause the conflict/are required

But if we revert something, we will need to re-revert it once we merge the whole branch right ? isn't weird ?

aabadie commented 6 years ago

Thanks @miri64. Is it ok if we cherrypick commits from #7658 in new_i2c_if ?

miri64 commented 6 years ago

But if we revert something, we will need to re-revert it once we merge the whole branch right ? isn't weird ?

Yes, but that's what we did for gnrc_netif/gnrc_ipv6_nib as well.

Thanks @miri64. Is it ok if we cherrypick commits from #7658 in new_i2c_if ?

I'd rather like you to open backport commits, so the history is trackable in both git and GitHub.

dylad commented 6 years ago

We need to take a decision about the I2C return code within the API. Should we return I2C_ACK (aka 0) or the number of bytes read/written ? I guess the latter may help when debugging. This is also the current behaviour of the SPI bus.

What do you think ?

aabadie commented 6 years ago

+1 for the number of bytes

dylad commented 6 years ago

Ok after some debate with myself, I change my mind.I think we should drop the number of bytes read/written. I2C is synchronous bus. We're not supposed to read more bytes by design and if we read/write less, the function will return an error code like I2C_DATA_NACK or I2C_ERR. I also took another look to the SPI implementation. The behaviour is not that clear to me and return the return is not always used (some functions return void)

MrKevinWeiss commented 6 years ago

I will just put some things I noticed regarding that issue:

dylad commented 6 years ago

Adding error codes to the API is something trivial as we can implement them later in the CPU drivers.

The only time the number of bytes would differ from length is an error (maybe you can think of adding timeout and collision errors), meaning that the number of bytes should never return any non error that is not equal to length

Some driver already implements timeout or lost arbitration on the bus. So I guess it's probably more useful to improve our error codes. If something is wrong on the bus, we will return the dedicated error code and if everything is alright, what's the point on returning the number of bytes written or read as we already knew it and pass it as an argument ?

Dropping the return number of bytes will force us to make more changes in devices driver but this is for our sake right ? :)

MrKevinWeiss commented 6 years ago

If something is wrong on the bus, we will return the dedicated error code and if everything is alright, what's the point on returning the number of bytes written or read as we already knew it and pass it as an argument?

Agreed, I don't think there should ever be cases that we wouldn't get an error but would have a different number of bytes.

Dropping the return number of bytes will force us to make more changes in devices driver but this is for our sake right?

Heh, I didn't think of it like that but it makes sense. Most things use this as 0 is an error, now that it is success it will be easier to detect. I guess the question is try to keep some compatibility which might have small unknown errors or make it blatant and propagate changes everywhere.

aabadie commented 6 years ago

RPi i2c c library seems to return the number of bytes (but I think it is because they use a filesystem write) but doesn't use error codes (so if error it a read will return 0)

I looked at the I2C api of other MCU OSes (Zephyr/MyNewt/Nuttx) and they all use return codes (0: success, negative values in case of errors, some use errno return codes). None are using the number of bytes read/written. So this goes into @dylad direction.

dylad commented 6 years ago

If we're agreed then we will take this direction. Using errno could be interesting but is this already used in RIOT ? I never saw it but it already covers 99% of use case. Most of the work is locate in the device driver. We can still add new I2C error code now (or after this refactoring) in the API, which is not harmful to any drivers and add support for this new error code in CPU driver. I will take a look at all currently implemented I2C driver today. Then we can discuss adding new error code (like timeout, lost arbitration etc.). If most of the previous CPU driver already support support it then we can add new error code, if not this will be done later as CPU driver improvement.

if you have any idea on useful error code, feel free to post it :) This idea is to use all of this to ease debugging.

aabadie commented 6 years ago

Using errno could be interesting but is this already used in RIOT ?

I think netdev, can, saul apis are using them and there are occurrences in the native related code.

What about -EIO (IO errors), -ENXIO (no such device or address), -ETIMEDOUT ?

dylad commented 6 years ago

So the current I2C_ADDR_NACK will be replaced by -ENXIO but what about I2C_DATA_NACK ? replaced by -EIO ?

MrKevinWeiss commented 6 years ago

+1 +1 +1 to errno!

jnohlgard commented 6 years ago

+1 for errno, but the common codes must be explained in the function docs (no data ACK, no addr ACK, timeout, and so on). Otherwise each cpu implementation is likely to use different codes for the same condition.

MrKevinWeiss commented 6 years ago

I think we need to rework the drivers/periph_common/i2c.c since the i2c_write_regs doesn't seem to support the typical i2c slave register access. Currently it is 

int i2c_write_regs(i2c_t dev, uint16_t addr, uint16_t reg, const void *data, size_t len, uint8_t flags)
{
    int8_t err;
    /* First set ADDR and register with no stop */
    err = i2c_write_bytes(dev, addr, &reg, (flags & I2C_REG16) ? 2 : 1, flags & I2C_NOSTOP );
    if (err < 0) {
        return err;
    }
    /* Then write data to the device */
    return i2c_write_bytes(dev, addr, data, len, flags);
}

which seems to do separate reads, what I think it should be is:

int i2c_write_regs(i2c_t dev, uint16_t addr, uint16_t reg, const void *data, size_t len, uint8_t flags)
{
    int8_t err;
    /* First set ADDR and register with no stop */
    err = i2c_write_bytes(dev, addr, &reg, (flags & I2C_REG16) ? 2 : 1, I2C_NOSTOP );
    if (err < 0) {
        return err;
    }
    /* Then write data to the device */
    return i2c_write_bytes(dev, addr, data, len, I2C_NOSTART);
}

I don't think we should allow users to set I2C_NOSTART or I2C_NOSTOP flags, only the I2C_REG16 and maybe the I2C_ADDR10.

jnohlgard commented 6 years ago

One of the requested features in a api rework is the ability to do repeated starts. There are weird devices which use multiple repeated starts as part of their protocol. I think the function you posted just needs to change flags & no stop to flags| no stop

dylad commented 6 years ago

@gebart is right, you are probably using an old version of the API because we saw this error and flags & I2C_NOSTOP has been replaced by flags | I2C_NOSTOP already.

MrKevinWeiss commented 6 years ago

I still think there may be an issue, what about if it was (flags | NOSTOP & ~NOSTART) since the first write must always write the start and the second write should either NOSTART for the majority of sensor or have a start in case of repeated start. Sound OK?

MrKevinWeiss commented 6 years ago

you are probably using an old version of the API

Changing so fast!

dylad commented 6 years ago

Changing so fast!

This is a fast growing child !

We can clear the NOSTART flag by default for the first call but I don't think we should change the second call. Don't forget theses functions are just here for basic, generic, and non-weird I2C sensors. If we implement weird I2C stuff later, the device driver will have to implement its own read/write function using i2c_read_bytes and i2c_write_bytes with the flags which give much more flexibility.

MrKevinWeiss commented 6 years ago

Can anyone think of a case where we would need I2C_NOSTOP or I2C_NOSTART with read_bytes command? If not than I think I got a bug free ;) solution that works with the SAMR21 and fits the API.

jnohlgard commented 6 years ago

It would be good to have periph_init run after uart_stdio has been initialized. My debug prints in init are causing the system to crash because of uninitialized stdio... I'll remove them for now

dylad commented 6 years ago

@gebart I'm not against, in fact that would be logical. But this doesn't belong to the I2C refactoring as they are plenty of way for doing it. Maybe opening an issue is a better idea. I don't think all maintainers/developers are looking at this issue.

dylad commented 6 years ago

I take a look at the Linux kernel documentation -> here I propose to use the following errno's code : -EIO -> I2C_DATA_NACK ?, -ENXIO -> equivalent to the current I2C_ADDR_NACK, -ETIMEDOUT -> timeout occurs before device's response -EOPNOTSUPP -> use when a flag or operation is not supported by the driver -EAGAIN -> lost bus arbitration (when another master take the lead of the bus)

What do you think ? Did I miss a case ?

jnohlgard commented 6 years ago

@dylad I have used -EINVAL in other places when there were invalid arguments passed to functions, such as flags. How about EINVAL instead of EOPNOTSUPP?

jnohlgard commented 6 years ago

This is a bit late, but changing the CPU periph api into three primitives would make it possible to reuse a larger part of the logic as periph_common instead of reimplementing in each CPU. The primitives would be start, transmit, and receive. Where the transmit or receive functions would perform a stop unless a flag is sent. This way we could place all of the logic for 10 bit addressing inside periph_common and the cpu drivers would not need to bother with anything but normal 7 bit addresses in the start primitive. Slave mode would still need special implementations for 10 to bit mode on each CPU though.

I don't really see any reason why a CPU driver needs to implement the _regs functions, the periph_common implementation should be equivalent. The register address is only a few bytes at most and the i2c bus is slow, so there is little to gain from streamlining it all with a single DMA transfer instead of splitting it in two transfers.

dylad commented 6 years ago

@gebart Yes we can use -EINVAL for invalid argument like the len params. But I'm septic for the flags. returning an EINVAL for a flag that we define in our own API is just to weird for me.

In the current state, CPU implements must implements at least read_bytes and write_bytes which are basically the transmit and receive you were talking. I don't see a particular reason to export the start function outside of the CPU driver since we have enough control of this using the flags.

All others I2C API functions have default wrappers under drivers/periph_common/i2c.c that are #ifdef If the CPU want to use the default wrapper then the arch must define it (like the SPI bus in fact). You can have an example here.

If for a specific reason the CPU cannot use the default wrapper (hardware limitation ? Sillicon bug ? Weird IP ?) then CPU driver can redefine each functions. So right know CPU drivers don't need to implement _regs functions but they can do it if it's needed.