Add busio.~~I2CSlave~~ (I2CPeripheral, later I2CTarget)

[notro]

This adds a busio.I2CSlave binding and implements it on samd.
M4 is not tested yet, I want to see if the API needs to change before spending time on that.

I didn't use asf4 because it lacked smbus support (and the code looked fragile wrt misbehaving masters).

I2CSlaveRequest has 3 readonly properties and I wonder if there's a simpler way to implement those than to use accessor functions as I have done.

How do I make I2CSlave optional on other ports? Use a macro?

Example use:

import board
import busio

regs = [0] * 16
index = 0

with busio.I2CSlave(board.SCL, board.SDA, (0x40, 0x41)) as slave:
    while True:
        r = slave.request()
        if not r:
            # Maybe do some housekeeping
            continue
        with r:  # Closes the transfer if necessary by sending a NACK or feeding the master dummy bytes
            if r.address == 0x40:
                if not r.is_read:  # Master write which is Slave read
                    b = r.read(1)
                    if not b or b[0] > 15:
                        break
                    index = b[0]
                    b = r.read(1)
                    if b:
                        regs[index] = b[0]
                elif r.is_restart:  # Combined transfer: This is the Master read message
                    n = r.write(bytes([regs[index]]))
                #else:
                    # A read transfer is not supported in this example
                    # If the Master tries, it will get 0xff byte(s) by the ctx manager (r.close())
            elif r.address == 0x41:
                if not r.is_read:
                    b = r.read(1)
                    if b and b[0] == 0xde:
                        # do something
                        pass

Use from Linux:

$ i2cget -y 1 0x40 0x01
0x00
$ i2cset -y 1 0x40 0x01 0xaa
$ i2cget -y 1 0x40 0x01
0xaa

I've started on a SMBUS slave library that also contains tests for I2CSlave: https://github.com/notro/cp-smbusslave
It supports non-standard sequential modes which makes it useful with some i2c devices.

Example use:

import board
import busio
from smbusslave import SMBusSlave

class Slave(SMBusSlave):
    def __init__(self):
        super().__init__()
        self.protocol = SMBusSlave.SMBUS_BYTE
        self.max_reg = 15
        self.regs = [0] * (self.max_reg + 1)

    def readreg(self, reg):
        return self.regs[reg]

    def writereg(self, reg, val):
        self.regs[reg] = val

bs = Slave()
with busio.I2CSlave(board.SCL, board.SDA, (0x40,), smbus=True) as slave:
    while True:
        r = slave.request()
        if r:
            with r:
                bs.process(r)

Currently there are 3 emulated devices:

• DS1307 - Real Time Clock
• ADS1015 - Analog to Digital Converter (need some more work)
• AT24Cxx - EEPROM

The first two are potentially useful with a Raspberry Pi since it lacks a hw clock and an ADC.

The i2c hw on the Pi doesn't support clock stretching so it's necessary to use bitbanging.
/boot/config.txt

dtoverlay=i2c-gpio,bus=1,i2c_gpio_sda=2,i2c_gpio_scl=3

[tannewt]

Thanks for working on this!

7k bytes seems like a lot! Split it into its own module and list it here: https://github.com/adafruit/circuitpython/blob/master/ports/atmel-samd/mpconfigport.h#L221

[notro]

Version 2

• Moved from busio to new module i2cslave
• Add code example to the docs
• Add warning in docs about Raspberry Pi needing the i2c-gpio driver and that a HAT eeprom can't be emulated.

Two things I didn't know where to put:

I2CSlaveRequest has 3 readonly properties: address, is_read and is_restart. I've implemented these using property getters.
If there's no simpler way of doing this, maybe this macro I made should go into a headerfile somewhere:

#define MP_DEFINE_CONST_PROP_GET(obj_name, fun_name) \
    const mp_obj_fun_builtin_fixed_t fun_name##_obj = {{&mp_type_fun_builtin_1}, .fun._1 = fun_name}; \
    const mp_obj_property_t obj_name = { \
        .base.type = &mp_type_property, \
        .proxy = {(mp_obj_t)&fun_name##_obj, \
                  (mp_obj_t)&mp_const_none_obj, \
                  (mp_obj_t)&mp_const_none_obj}, }

I've made this function to honour Ctrl-C in the REPL. Where should I put it?

static inline bool mp_hal_is_interrupted(void) {
    #ifdef MICROPY_VM_HOOK_LOOP
        MICROPY_VM_HOOK_LOOP
    #endif
    return MP_STATE_VM(mp_pending_exception) == MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_kbd_exception));
}

[tannewt]

This could go into the peripherals library. Up to you if you want to move it.

[tannewt]

I'd put the macro here: https://github.com/adafruit/circuitpython/blob/master/py/obj.h#L295

I'd put the helper method here: https://github.com/adafruit/circuitpython/blob/master/lib/utils/interrupt_char.c#L49

Thanks!

[notro]

Version 3

• Rebased
• Move mp_hal_is_interrupted() to lib/utils/interrupt_char.c
• Move MP_DEFINE_CONST_PROP_GET to py/obj.h
• Now also tested on M4

These ran out of space:

circuit_playground_express
/usr/bin/../lib/gcc/arm-none-eabi/7.3.1/../../../../arm-none-eabi/bin/ld: region `FLASH' overflowed by 5304 bytes

circuit_playground_express_crickit
/usr/bin/../lib/gcc/arm-none-eabi/7.3.1/../../../../arm-none-eabi/bin/ld: region `FLASH' overflowed by 4776 bytes

feather_m0_express_crickit
/usr/bin/../lib/gcc/arm-none-eabi/7.3.1/../../../../arm-none-eabi/bin/ld: region `FLASH' overflowed by 5480 bytes

hallowing_m0_express
/usr/bin/../lib/gcc/arm-none-eabi/7.3.1/../../../../arm-none-eabi/bin/ld: region `FLASH' overflowed by 224 bytes

[notro]

The feather_huzzah build choked on my bool:

In file included from ./esp_mphal.h:28:0,
                 from ../../py/mphal.h:32,
                 from ../../py/mpprint.c:33:
../../lib/utils/interrupt_char.h:32:1: error: unknown type name 'bool'
 bool mp_hal_is_interrupted(void);
 ^

I'll include stdbool.h in the next version when the out of flash solution is decided.

[tannewt]

Hey @notro, mind adding a shared define such as SPECIALTY_BOARD that will trigger the exclusion of I2CSlave? Those four builds are full because they include a bunch of library code for their specific uses so it doesn't need to be loaded into RAM.

[notro]

Is this what you had in mind?

     #if defined(__SAMD51G19A__) || defined(__SAMD51G18A__)
         #define AUDIOBUSIO_MODULE
     #else
         #define AUDIOBUSIO_MODULE { MP_OBJ_NEW_QSTR(MP_QSTR_audiobusio), (mp_obj_t)&audiobusio_module },
     #endif

+    #ifdef SPECIALTY_BOARD
+        #define I2CSLAVE_MODULE
+    #else
+        #define I2CSLAVE_MODULE { MP_OBJ_NEW_QSTR(MP_QSTR_i2cslave), (mp_obj_t)&i2cslave_module },
+    #endif
+
     #ifndef EXTRA_BUILTIN_MODULES
     #define EXTRA_BUILTIN_MODULES \
         { MP_OBJ_NEW_QSTR(MP_QSTR_audioio), (mp_obj_t)&audioio_module }, \
         AUDIOBUSIO_MODULE \
         { MP_OBJ_NEW_QSTR(MP_QSTR_bitbangio), (mp_obj_t)&bitbangio_module }, \
+        I2CSLAVE_MODULE \
         { MP_OBJ_NEW_QSTR(MP_QSTR_rotaryio), (mp_obj_t)&rotaryio_module }, \
         { MP_OBJ_NEW_QSTR(MP_QSTR_gamepad),(mp_obj_t)&gamepad_module }
     #endif
     #define EXPRESS_BOARD

[dhalbert]

How about just CIRCUITPY_I2CSLAVE instead of SPECIALTY_BOARD, since there will probably be other things later that we want to enable or disable?

I am also surprised it is 7kB. Have you looked at the firmware.elf.map to see what's taking up so much space? Is it all just the new C code you wrote?

Besides the map, this command is useful for figuring out what's taking up space in the the image:

arm-none-eabi-nm --print-size --size-sort --reverse-sort firmware.elf

And this script called on the firmware.elf gives caller-callee relationships:

#!/bin/bash
# https://reverseengineering.stackexchange.com/questions/9113/how-can-i-generate-a-call-graph-from-an-unstripped-x86-
linux-elf
arm-none-eabi-objdump -d $1 \
| grep '<' \
| sed -e 's/^[^<]*//' \
| sed 's/<\([^+]*\)[^>]*>/\1/' \
| awk 'BEGIN { FS = ":" } \
       NF>1 { w=$1; } \
       NF==1 && w != $1 { print "(\"" w "\", \"" $0 "\")," }' \
| sort -u

[notro]

Yes it's just the C code.

This didn't make much sense to me, but here they are:

• firmware.elf.map
• arm-none-eabi-nm
• call_graph

[dhalbert]

Thanks, that's very helpful. What I see is that the double-precision floating point library has been dragged in, because I see _aeabi_dadd, _aeabi_dsub, etc. These routines are large, and you have to be careful to avoid invoking them implicitly.

They were brought in probably because of this code:
https://github.com/notro/circuitpython/blob/dccf8f7523cc82c67a48c2b627709bd484fca90e/shared-bindings/i2cslave/I2CSlave.c#L170-L177, probably specifically line 174

    uint64_t timeout_end;
    if (timeout < 0) {
        timeout_end = 0;
    } else if (timeout > 0) {
        timeout_end = common_hal_time_monotonic() + timeout * 1000;   // <--THIS LINE USES double arith
    } else {
        timeout_end = ~0; // "infinity"
    }

which is mixing uint64_t and a float. Try reworking this by making timeout_end be a float, or using a 32-bit delta, or similar. You'll know you succeeded when _aeabi_dadd et al disappear from firmware.elf.map, and the code size shrinks a lot.

See #342 and #343 for an example of what I did to remove the double libraries in another case with uint64_t.

[notro]

Thanks @dhalbert for catching this.

Changing timeout to timeout_ms and always use an integer avoids pulling in the library and the size increase is now ~2.8k:

     #if MICROPY_PY_BUILTINS_FLOAT
-    float timeout = mp_obj_get_float(args[ARG_timeout].u_obj);
+    float f = mp_obj_get_float(args[ARG_timeout].u_obj) * 1000;
+    int timeout_ms = (int)f;
     #else
-    int timeout = mp_obj_get_int(args[ARG_timeout].u_obj);
+    int timeout_ms = mp_obj_get_int(args[ARG_timeout].u_obj) * 1000;
     #endif

I discovered a problem with the mp_hal_is_interrupted() I made.

// Check to see if we've been CTRL-C'ed by autoreload or the user.
bool mp_hal_is_interrupted(void) {
    #ifdef MICROPY_VM_HOOK_LOOP
        MICROPY_VM_HOOK_LOOP
    #endif
    return MP_STATE_VM(mp_pending_exception) == MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_kbd_exception));
}

If I press Ctrl-C once in the REPL, it will return true on all subsequent calls. The exception is not reset. Is it possible to fix that?

>>> import board
>>> from i2cslave import I2CSlave
>>> slave = I2CSlave(board.SCL, board.SDA, (0x40,))

This waits forever:
>>> print(slave.request(timeout=0))
<PRESS CTRL-C>
None

This should also wait forever but returns immediately:
>>> print(slave.request(timeout=0))
None

I see that the same happens when time.sleep() is interrupted.

[notro]

So the Ctrl-C problem is of recent origin: #1092

Back to flash overflow. The Hallowing builds now, but these don't:

circuit_playground_express
region `FLASH' overflowed by 320 bytes

circuit_playground_express_crickit
region `FLASH' overflowed by 984 bytes

feather_m0_express_crickit
region `FLASH' overflowed by 556 bytes

@tannewt suggested a SPECIALTY_BOARD macro to turn off i2cslave #if BOARD_FLASH_SIZE > 192000.
@dhalbert suggested a CIRCUITPY_I2CSLAVE macro to turn on i2cslave.
To me it doesn't matter either way.

One difference is whether i2cslave is built by default or not.

[dhalbert]

One question is whether someone who needs I2C slave is savvy enough to know how to do builds. Are you thinking of an SPI slave also?

If we turn it on in general, I'd turn it on for the general purpose Express boards and all M4 boards (but I know it's not implemented there yet). Leave it off for the non-express boards and turn it off for CPX and the *_crickit builds, since they have a lot of frozen code and their users are less likely to need this feature. So it would be #ifdef-guarded and turned on in mpconfigport.h #if BOARD_FLASH_SIZE > 192000. You can use boards/*/mpconfigboard.h` to turn it off for CPX and the crickit builds.

[notro]

Are you thinking of an SPI slave also?

No.

The reason I'm doing I2CSlave is so I can emulate an RTC (ds1307) and PMIC (axp209) for use with a Raspberry Pi. The Pi lacks a clock and a power controller.
It will also give the Pi access to an ADC (which it doesn't have) that I might use with a light sensor to control display brightness.

[tannewt]

@dhalbert I wonder if its worth adding a grep over the map file to ensure its not added again. Thanks for catching it!

@notro I'm fine with CIRCUITPY_I2CSLAVE.

[notro]

Version 4

• Import stdbool.h in lib/utils/interrupt_char.h
• Fix bug where Ctrl-C in I2CSlave.request() could result in raising OSError
• Avoid dragging in the double-precision floating point library (thanks @dhalbert)
• Use macro CIRCUITPY_I2CSLAVE to enable i2cslave
• i2cslave is enabled on: feather_m0_express, feather_m4_express, metro_m0_express and metro_m4_express
• Rebase on i18n

[notro]

I have a problem with the MP_ETIMEDOUT value. I expect it to be 110 but it is 116:

This is the C code:

    if (timeout_ms > 0) {
        mp_raise_OSError(MP_ETIMEDOUT);
    }

This is the result:

>>> slave.request(timeout=1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: 116
>>>

CPython value:

>>> errno.ETIMEDOUT
110

From py/mperrno.h (MICROPY_USE_INTERNAL_ERRNO is 0):

#if MICROPY_USE_INTERNAL_ERRNO

// MP_Exxx errno's are defined directly as numeric values
// (Linux constants are used as a reference)

<snip>
#define MP_ETIMEDOUT        (110) // Connection timed out
<snip>

#else

// MP_Exxx errno's are defined in terms of system supplied ones

#include <errno.h>

<snip>
#define MP_ETIMEDOUT        ETIMEDOUT
<snip>

#endif

Does anyone know where the file #include <errno.h> is?

[dhalbert]

In arm-none-eabi/include/sys/errno.h in the toolchain ETIMEDOUTis defined as 116. In Linux it's 110:/usr/include/libr/sflib/common/sftypes.h(which is included indirectly byerrno.h`, it appears). It's 116 in FreeRTOS and Android.

This already came up in micropython#976. We could set MICROPY_USE_INTERNAL_ERRNO to (1) in our ports. It is set to 0 by default but is set to 1 for st32, esp32, and cc3200. I'm not sure it matters as long as we are consistent.

[notro]

Thanks @dhalbert, so it's a OS issue then.

[tannewt]

Looks like the huzzah build is missing in include. Everything else looks good to go.

[notro]

It's not obvious to me what to include.

lib/utils/interrupt_char.c

#include "py/obj.h"
#include "py/mpstate.h"

#if MICROPY_KBD_EXCEPTION

<snip>

// Check to see if we've been CTRL-C'ed by autoreload or the user.
bool mp_hal_is_interrupted(void) {
    #ifdef MICROPY_VM_HOOK_LOOP
        MICROPY_VM_HOOK_LOOP
    #endif
    return MP_STATE_VM(mp_pending_exception) == MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_kbd_exception));
}

#endif

ports/esp8266/mpconfigport.h

extern void ets_event_poll(void);
#define MICROPY_EVENT_POLL_HOOK {ets_event_poll();}
#define MICROPY_VM_HOOK_COUNT (10)
#define MICROPY_VM_HOOK_INIT static uint vm_hook_divisor = MICROPY_VM_HOOK_COUNT;
#define MICROPY_VM_HOOK_POLL if (--vm_hook_divisor == 0) { \
        vm_hook_divisor = MICROPY_VM_HOOK_COUNT; \
        extern void ets_loop_iter(void); \
        ets_loop_iter(); \
    }
#define MICROPY_VM_HOOK_LOOP MICROPY_VM_HOOK_POLL
#define MICROPY_VM_HOOK_RETURN MICROPY_VM_HOOK_POLL

Build error:

xtensa-lx106-elf-gcc -I../../lib/berkeley-db-1.xx/PORT/include -I. -I../.. -Ibuild -I/home/travis/build/adafruit/circuitpython/xtensa-lx106-elf/bin/../xtensa-lx106-elf/sysroot/usr/include -Wall -Wpointer-arith -Werror -Wno-strict-aliasing -std=gnu99 -nostdlib -DUART_OS=0 -fsingle-precision-constant -Wdouble-promotion -D__ets__ -DICACHE_FLASH -fno-inline-functions -Wl,-EL -mlongcalls -mtext-section-literals -mforce-l32 -DLWIP_OPEN_SRC -DFFCONF_H=\"lib/oofatfs/ffconf.h\" -DMICROPY_PY_USSL=1 -DMICROPY_SSL_AXTLS=1 -I../../lib/axtls/ssl -I../../lib/axtls/crypto -I../../lib/axtls/config -DMICROPY_PY_BTREE=1 -Os -DNDEBUG  -fdata-sections -ffunction-sections   -c -o build/frozen.o build/frozen.c
In file included from ../../py/mpconfig.h:45:0,
                 from ../../py/obj.h:31,
                 from ../../lib/utils/interrupt_char.c:27:
../../lib/utils/interrupt_char.c: In function 'mp_hal_is_interrupted':
./mpconfigport.h:120:36: error: 'vm_hook_divisor' undeclared (first use in this function)
 #define MICROPY_VM_HOOK_POLL if (--vm_hook_divisor == 0) { \
                                    ^
./mpconfigport.h:125:30: note: in expansion of macro 'MICROPY_VM_HOOK_POLL'
 #define MICROPY_VM_HOOK_LOOP MICROPY_VM_HOOK_POLL
                              ^
../../lib/utils/interrupt_char.c:53:9: note: in expansion of macro 'MICROPY_VM_HOOK_LOOP'
         MICROPY_VM_HOOK_LOOP
         ^
./mpconfigport.h:120:36: note: each undeclared identifier is reported only once for each function it appears in
 #define MICROPY_VM_HOOK_POLL if (--vm_hook_divisor == 0) { \
                                    ^
./mpconfigport.h:125:30: note: in expansion of macro 'MICROPY_VM_HOOK_POLL'
 #define MICROPY_VM_HOOK_LOOP MICROPY_VM_HOOK_POLL
                              ^
../../lib/utils/interrupt_char.c:53:9: note: in expansion of macro 'MICROPY_VM_HOOK_LOOP'
         MICROPY_VM_HOOK_LOOP
         ^
make: *** [build/lib/utils/interrupt_char.o] Error 1
make: *** Waiting for unfinished jobs....
make: Leaving directory `/home/travis/build/adafruit/circuitpython/ports/esp8266'

[tannewt]

Ugh ya. Some files simply don't include what they use. That's why I thought it would be simple.

Here I think we should move the VM_HOOK part back into the wait loops. ESP doesn't use that code anyway so it should fix the compile.

[notro]

Ok, I'll take it out of mp_hal_is_interrupted().

Is it really necessary to check that the macro is defined:

    #ifdef MICROPY_VM_HOOK_LOOP
        MICROPY_VM_HOOK_LOOP
    #endif

When we have this in py/mpconfig.h?

// Hook for the VM during the opcode loop (but only after jump opcodes)
#ifndef MICROPY_VM_HOOK_LOOP
#define MICROPY_VM_HOOK_LOOP
#endif

It looks like I can just do this:

    while (common_hal_time_monotonic() < timeout_end) {
        MICROPY_VM_HOOK_LOOP
        if (mp_hal_is_interrupted()) {
            break;
        }

Actually that's how it's used in py/vm.c:

pending_exception_check:
                MICROPY_VM_HOOK_LOOP

main.c and the samd and nrf ports check the macro before using it.

[tannewt]

It might just be a legacy thing. Feel free to remove the ifdef.

[notro]

Version 5

• Move MICROPY_VM_HOOK_LOOP out of mp_hal_is_interrupted() and into the respective loops
• Rebase

[tannewt]

Looks great! Thank you!

[notro]

Thanks for your review @tannewt!