microbit-foundation / micropython-microbit-v2

Temporary home for MicroPython for micro:bit v2 as we stablise it before pushing upstream
MIT License
45 stars 25 forks source link

i2c.scan #55

Closed microbit-carlos closed 3 years ago

microbit-carlos commented 3 years ago

From https://github.com/bbcmicrobit/micropython/issues/699#issuecomment-753816394 (thanks @xmeow!):

@carlosperate Thanks for advise the correct repo for micropython firmware. The only issue is that the latest beta.3 firmware may output unambiguous devices that don't exist.

MicroPython v1.13 on 2020-12-21; micro:bit v2.0.0-beta.3 with nRF52833
Type "help()" for more information.
>>> i2c.scan()
[9, 12, 15, 17, 19, 21, 23, 26, 29, 31, 33, 35, 37, 39, 42, 44, 47, 49, 51, 53, 55, 58, 60, 63, 65, 66, 68, 71, 73, 75, 77, 79, 81, 83, 86, 88, 91, 93, 96, 98, 100, 102, 104, 107, 109, 111, 113, 114, 117, 119]
>>>

I tried to build the firmware locally and change the last stop flag to false and got the correct output.

>>> MicroPython v1.13-dirty on 2021-01-04; micro:bit v2.0.0-beta.3 with nRF52833
Type "help()" for more information.
>>>
>>> i2c.scan()
[64, 112]

It looks like the last param should be named repeat instead of stop.

STATIC mp_obj_t microbit_i2c_scan(mp_obj_t self_in) {
    mp_obj_t list = mp_obj_new_list(0, NULL);
    // 7-bit addresses 0b0000xxx and 0b1111xxx are reserved
    for (int addr = 0x08; addr < 0x78; ++addr) {
        int ret = microbit_hal_i2c_writeto(addr, NULL, 0, false); // true to flase
dpgeorge commented 3 years ago

It looks like the last param should be named repeat instead of stop.

I'm quite sure the last parameter to microbit_hal_i2c_writeto() is "stop" -- this value is actually inverted in the implementation of this function, when calling the corresponding CODAL function which names it "repeated".

Maybe we should try to write an I2C scan implementation in pure C++ using the CODAL directly, to see how that would work.

microbit-mark commented 3 years ago

This has also been raised by another person in micro:bit support https://support.microbit.org/a/tickets/42809

microbit-carlos commented 3 years ago

The detected addresses are not constant:

>>> i2c.scan()
[9, 12, 14, 16, 18, 20, 23, 25, 28, 30, 33, 34, 36, 38, 41, 43, 45, 47, 49, 51, 53, 56, 58, 60, 62, 64, 66, 69, 71, 74, 76, 78, 80, 82, 84, 86, 89, 91, 93, 96, 98, 101, 103, 105, 108, 110, 112, 114, 116, 118]
>>> i2c.scan()
[9, 12, 14, 16, 18, 21, 23, 25, 27, 29, 31, 34, 36, 38, 41, 43, 45, 48, 51, 53, 55, 57, 59, 61, 64, 66, 69, 71, 74, 77, 79, 81, 83, 85, 87, 89, 91, 94, 96, 99, 101, 103, 105, 108, 110, 112, 114, 117, 119]
>>> i2c.scan()
[9, 11, 14, 17, 19, 21, 23, 26, 29, 31, 33, 35, 37, 40, 43, 45, 48, 50, 53, 56, 58, 60, 62, 64, 66, 69, 71, 74, 76, 78, 81, 83, 86, 88, 90, 92, 95, 97, 99, 101, 103, 106, 108, 110, 112, 115, 117, 119]
>>> i2c.scan()
[9, 11, 14, 16, 18, 20, 22, 24, 26, 28, 30, 33, 35, 37, 39, 42, 44, 46, 48, 51, 53, 56, 59, 61, 64, 66, 69, 71, 73, 75, 78, 80, 82, 84, 87, 89, 91, 94, 96, 98, 101, 103, 106, 108, 110, 113, 115, 118]
>>> i2c.scan()
[9, 12, 14, 16, 18, 21, 23, 26, 28, 30, 32, 35, 37, 40, 43, 45, 48, 50, 53, 55, 58, 60, 62, 64, 66, 68, 70, 73, 75, 78, 80, 82, 84, 87, 89, 91, 93, 96, 99, 101, 104, 107, 109, 112, 114, 117]

This ticket in the codal repo contains more info: https://github.com/lancaster-university/codal-microbit-v2/issues/49

dpgeorge commented 3 years ago

As mentioned over at https://github.com/lancaster-university/codal-microbit-v2/issues/49 I can work around this issue by adding a small delay (50us) after the uBit.i2c.write() call. But otherwise I suspect this is a problem with the CODAL.

jaustin commented 3 years ago

There's a new build of the DAL that fixes scan behavior, so we should test this against that (de856f2 has the fix in)

dpgeorge commented 3 years ago

There's a new build of the DAL that fixes scan behavior, so we should test this against that (de856f2 has the fix in)

I tried to test it but didn't see any change in I2C behaviour, an i2c.scan() still returns a list of many random values.

I looks like the only relevant change for I2C is the commit which enables NRF52I2C_ERRATA_219 in target.json. But when I builds for me it uses target-locked.json so I added that errata config to target-locked.json as well. Still it did not do anything (I did a clean build with this change but not sure it actually applied it... hard to tell).

If someone else can confirm this (maybe try the C++ example) that would be great.

microbit-carlos commented 3 years ago

Yeah, the main changes ended up in the codal-nrf52 repo. Testing a build from the current MicroPython master branch and I can still replicate the issue. Then git pulling the latest codal-nrf52 (https://github.com/lancaster-university/codal-nrf52/commit/86be5f29342b2bac245d42ad26f81a4e7fde2c69 )and codal-microbit-v2 (https://github.com/lancaster-university/codal-microbit-v2/commit/de856f22f7fbea1145c24fff7933dd12fc30b2ae):

$ cd lib/codal/libraries/codal-nrf52
$ git pull origin master
$ cd ../codal-microbit-v2
$ git pull origin master

And the resulting hex i2c.scan works (tested with and without I2C devices on the edge connector) 🎉 MICROBIT-i2c.hex.zip

dpgeorge commented 3 years ago

Then git pulling the latest codal-nrf52 and codal-microbit-v2 ...

Yes, doing this I can now confirm the fix, great!

I guess we just need to wait now for a tag on codal-microbit-v2?

dpgeorge commented 3 years ago

CODAL was updated in 76054c2e9445d8eea285132c2374a18ddc690514 so hopefully this is fixed now.

microbit-carlos commented 3 years ago

Can confirm this is fixed and will be present in the beta.5 release, thanks Damien!