Add support for MicroBitI2C.set/get_frequency(). #296

komeP · 2016-06-03T16:06:39Z

The frequency is clamped and stored in the mbed I2C object, but the set
function returns void and the storage is protected (and neither mbed nor DAL
provides a getter). I work around this with our own copy of the frequency and
our own validation code that raises ValueError.

I don't have equipment at PyCon to test this. Someone please make sure this works. The three frequencies that need to be tested are 100 KHz, 250 KHz, and 400 KHz.

Resolves #228

The frequency is clamped and stored in the mbed I2C object, but the set function returns void and the storage is protected (and neither mbed nor DAL provides a getter). I work around this with our own copy of the frequency and our own validation code that raises ValueError.

carlosperate · 2016-06-03T16:21:36Z

Last time I tried to do this, on this exact way, the frequency wasn't changing. So from that experience I assumed the DAL would have to be updated to expose their internal I2C object frequency method, but micropython is currently using an old version, and it would have to be retroactively patched. Because of that the fact that it would have to go from version 1.4.15 to probably 1.4.18 and I didn't know if there were other unwanted changes in-between I didn't really get around going that route.

I'll try to test your PR tonight just in case anything I might have just made a mistake on my version.

komeP · 2016-06-03T16:47:01Z

The DAL has no direct concept of I2C frequency. The frequency method I call is a public method directly from the mbed API's I2C class, which the DAL's MicroBitI2C class publicly inherits from. The frequency method saves the value to the protected _hz member and calls the HAL function i2c_frequency().

carlosperate · 2016-06-03T16:53:55Z

I know, that's why I was so surprised it didn't work when I tried it from micropython, maybe I made a mistake somewhere though, it was the first time I was playing around with the code. Worked fine when I added the public method on the DAL and called that though.

I'm on my way home now, I have a microbit all set up there, so I could soon get some results from this patch.

carlosperate · 2016-06-03T17:20:58Z

I can confirm it works a treat :)
At 400k it looks barely on spec (specially that stop bit), but the accelerometer still reads data fine. It would have been better if there was a lower value pull up. Does anybody know if that pull is external or internal?

komeP · 2016-06-03T17:23:16Z

Thank you for the test. I wonder if I should get a portable logic analyzer/scope...

carlosperate · 2016-06-03T17:24:56Z

At home I use a DSO, don't really have the space for a lab and can usually stay late at work if I need to use some proper equipment, but it's been really useful, I do recommend it.

carlosperate · 2016-06-03T17:31:59Z

As a general note, I'm not a big fan of having a get function for a locally saved value of what we expect the frequency to be. If any of the lower level layers changes the behaviour, or a different component decides to edit the frequency for whatever reason, then you'd be reporting an incorrect value.
Apart from that, I don't see many use cases where a user really needs to know the I2C frequency, generally speaking you just set it to the value you need without checking, so we could just save a tiny bit of space (plus some strings and the additional code) by not having it.

komeP · 2016-06-03T17:50:21Z

@ntoll and @markshannon concur. get_frequency() removed.

carlosperate · 2016-06-03T18:02:44Z

If there is not get_frequency then should set_frequency probably be renamed to just frequency?

carlosperate · 2016-06-03T18:04:17Z

source/microbit/microbiti2c.cpp

@@ -111,7 +141,7 @@ const mp_obj_type_t microbit_i2c_type = {

 const microbit_i2c_obj_t microbit_i2c_obj = {
    {&microbit_i2c_type},
-    .i2c = &uBit.i2c
+    .i2c = &uBit.i2c,


You can revert the command the line would then be unchanged.

komeP · 2016-06-03T18:12:45Z

I feels like it should be a property if it's named frequency.

carlosperate · 2016-06-03T18:15:10Z

Good point, also there are plenty of get_ functions, so good to follow convention.

dpgeorge · 2016-06-03T18:36:36Z

The uart object uses the init() method to set parameters like baudrate, and the I2C object should follow suit, eg:

i2c.init(freq=250000)

dpgeorge · 2016-06-03T18:47:44Z

The SPI object also uses init(). UART and SPI both use "baudrate" for their clock speed.

komeP · 2016-06-03T18:56:59Z

Using init() makes sense. However, I have never heard "baudrate" used in the context of I2C. NXP refers to this property as "clock frequency", so I think I will keep it "frequency" (spelling out "frequency" instead of "freq" is consistent with the music module).

carlosperate · 2016-06-03T23:19:16Z

source/microbit/help.c

@@ -132,7 +132,7 @@ STATIC const mp_doc_t help_table_instances[] = {
    {&microbit_pin_is_touched_obj, "If pin is_touched() on micro:bit, return True. If nothing is touching the pin,\nreturn False.\n"},
    // I2C
    {&microbit_i2c_obj, "Communicate with one or more named devices connected to micro:bit. Each named\ndevice has an 'address', communicates using I2C, and connects to the I/O pins.\n"},
-    {&microbit_i2c_set_frequency_obj, "Use set_frequency(f) to set the I2C bus frequency.\n"},
+    {&microbit_i2c_init_obj, "Use init() to set up communication with a clock frequench of 100 KHz.\nOverride the default for 'frequency' in Hz.\n"},


I quite like the spi init description Use init() to set up communication. Override the defaults for baudrate, mode,\nSCLK, MOSI and MISO. The default connections are pin13 for SCLK, pin15 for\nMOSI and pin14 for MISO.\n, makes it clear, in a concise form, that you can call without arguments or overwrite defaults. So maybe something slightly shorter like?:

Use init() to set up communication. Override the 100KHz default for 'frequency' with a value in Hz."

whaleygeek · 2016-06-04T09:08:51Z

So, if I have two devices on the bus (i2c or spi) that both require different clock frequencies, how do I switch between them without calling init()

I raise this, because I designed a product a couple of years ago (that is now successfully deployed in the field) that had a number of devices on the bus, each with different max freq requirements. To get the necessary performance out of the bus (so that the top loop worked in a sensible time and all sensors were acquired in a deadline), I had to live shift the clock frequency of the bus.

Do you think that calling init() regularly round a top loop will work fine in this scenario?

Some A2D converters for example have clock speed requirements that if not met, the on board charge reservoir capacitors aren't correctly maintained and you get linearisation errors in the readings across the scale.

carlosperate · 2016-06-04T11:54:49Z

At the moment for I2C the init function basically calls the c++ i2c frequency method, so you can safely call it at any point to switch frequencies and it should not affect any other properties of the peripheral.

However, if I am not mistaken, the DAL will be continuously updating things on the background that are using the i2c communication, so it that will obviously be affected by this.

carlosperate · 2016-06-04T12:01:34Z

As a continuation, I think the background updates are possibly only triggered the first time the DAL object is accessed from your program. So, for instance, if the microbit.accelerometer is never used, it should in theory not have this background updates running. But I've never really checked, so I might be wrong.

komeP · 2016-06-04T13:41:31Z

@markshannon explained to me that micropython disables the DAL event loop and installs its own, and that the accelerometer is read on-demand. The compass is also generally read on-demand, but will be automatically polled if it is still calibrating. I believe that we then have implicit bus locking in that the accelerometer and compass are generally synchronous. The investigation of the clock frequency situation however is still valid (and not a problem (yet)).

If the DAL does indeed perform background updates, then I2C support is truly a hackers-only feature since there is no guarantee that communications will not be interleaved at this point. We would either need to

~~Introduce bus locking to the DAL,~~
~~Disable the DAL's background updates entirely and require the Python user to perform these updates manually, or~~
~~Instruct the user to never call certain built-in modules and instead provide their own Python-level drivers as necessary.~~
I believe any of these three should be acceptable since I2C is a very advanced feature given the primary target audience of this system. Without modification to the DAL, we are stuck with the third option.

Upon inspection of the DAL code, they appear to always update after initialization of the C++ object. The constructors call configure(), which in both (accelerometer, compass) cases initialize their respective component with a sample rate. The idle loop callbacks poll the interrupt lines and are permanently registered in MicroBit::init(), so I would believe that the system begins reading these devices from before the REPL is ready.

However, as far as frequency is concerned, there is no changing (or setting) of the frequency in the DAL, and I could believe that the devices both support a 400 KHz bus, so for now we should be safe. Inspection of the datasheets for the two components shows that this is in fact true.

deshipu · 2016-06-04T22:20:07Z

You can't switch the bus frequencies to talk to different devices.
Unlike SPI, which uses a chip select pin to tell the devices when they
should listen to the data on the bus, I2C makes all devices listen all
the time, and ignore the data that is not addressed to them. In
practice that means that if you have at least one Standard-mode
(100kbit/s) device on your bus, all communication must be with that
speed, even when talking to faster devices, otherwise that slow device
may randomly interpret some of the data it manages to read as addressed
to itself and you get unpredictable behavior.

From the I2C specification:

Fast-mode devices are downward-compatible and can communicate with
Standard-mode devices in a 0 to 100 kbit/s I2C-bus system. As Standard-mode
devices, however, are not upward compatible; they should not be incorporated in
a Fast-mode I2C-bus system as they cannot follow the higher transfer rate and
unpredictable states would occur.

So it doesn't make sense to switch the I2C bus speed more than once, at the
beginning of your program. You should just set it to the speed of your
slowest device, and leave it like that.

On Sat, 04 Jun 2016 02:08:53 -0700
David Whale notifications@github.com wrote:

So, if I have two devices on the bus (i2c or spi) that both require
different clock frequencies, how do I switch between them without
calling init()

I raise this, because I designed a product a couple of years ago
(that is now successfully deployed in the field) that had a number of
devices on the bus, each with different max freq requirements. To get
the necessary performance out of the bus (so that the top loop worked
in a sensible time and all sensors were acquired in a deadline), I
had to live shift the clock frequency of the bus.

Do you think that calling init() regularly round a top loop will work
fine in this scenario?

Some A2D converters for example have clock speed requirements that if
not met, the on board charge reservoir capacitors aren't correctly
maintained and you get linearisation errors in the readings across
the scale.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#296 (comment)

Radomir Dopieralski

deshipu · 2016-06-04T22:21:32Z

You can't switch the bus frequencies to talk to different devices.
Unlike SPI, which uses a chip select pin to tell the devices when they
should listen to the data on the bus, I2C makes all devices listen all
the time, and ignore the data that is not addressed to them. In
practice that means that if you have at least one Standard-mode
(100kbit/s) device on your bus, all communication must be with that
speed, even when talking to faster devices, otherwise that slow device
may randomly interpret some of the data it manages to read as addressed
to itself and you get unpredictable behavior.

From the I2C specification:

Fast-mode devices are downward-compatible and can communicate with
Standard-mode devices in a 0 to 100 kbit/s I2C-bus system. As
Standard-mode devices, however, are not upward compatible; they should
not be incorporated in a Fast-mode I2C-bus system as they cannot follow
the higher transfer rate and unpredictable states would occur.

So it doesn't make sense to switch the I2C bus speed more than once, at
the beginning of your program. You should just set it to the speed of
your slowest device, and leave it like that.

On Sat, 04 Jun 2016 02:08:53 -0700
David Whale notifications@github.com wrote:

So, if I have two devices on the bus (i2c or spi) that both require
different clock frequencies, how do I switch between them without
calling init()

I raise this, because I designed a product a couple of years ago
(that is now successfully deployed in the field) that had a number of
devices on the bus, each with different max freq requirements. To get
the necessary performance out of the bus (so that the top loop worked
in a sensible time and all sensors were acquired in a deadline), I
had to live shift the clock frequency of the bus.

Do you think that calling init() regularly round a top loop will work
fine in this scenario?

Some A2D converters for example have clock speed requirements that if
not met, the on board charge reservoir capacitors aren't correctly
maintained and you get linearisation errors in the readings across
the scale.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#296 (comment)

Radomir Dopieralski

whaleygeek · 2016-06-04T23:08:23Z

Sure, that is true about I2C you are correct. But there is often a requirement to speed switch on SPI where multiple devices are in use. It might be clearer for there to be a consistent API across both of the busses? Or at least to make it possible to speed switch SPI live.

dpgeorge · 2016-06-05T12:28:16Z

But there is often a requirement to speed switch on SPI where multiple devices are in use.

Yes this is definitely true. Also changing the polarity/phase of the SPI bus between transfers is just as necessary as changing the frequency.

It might be clearer for there to be a consistent API across both of the busses? Or at least to make it possible to speed switch SPI live.

Using an init() method makes it consistent across UART, SPI and I2C. But we'd need to define the init() function to only update the given parameters (eg spi.init(phase=0) doesn't change anything except the phase). Unfortunately this is in conflict with how uart.init() works, which resets everything to the default if it's not specified as an argument.

carlosperate · 2016-06-06T11:18:31Z

But we'd need to define the init() function to only update the given parameters (eg spi.init(phase=0) doesn't change anything except the phase)

I personally find the current UART init behaviour a bit more intuitive due to the method name itself and the fact that the documentation clearly states defaults. If the function was called something like "configure" or "setup" I think I could assume that spi.configure(phase=0) could be leaving the other attributes unchanged, but seeing spi.init(phase=0) to me suggests all the other values are set to the default as the module is being "initialised to something". However this is a very individual perspective, and it is likely that other people will see this differently.

dpgeorge · 2016-06-06T12:12:52Z

I think we need to have a broader discussion wrt setting parameters, to make sure things are consistent. Things that are configurable are:

GPIO: set pull (see add set_pull method to pin class #304), set analog period
I2C: set frequency (see Add support for MicroBitI2C.set/get_frequency(). #296), change scl/sda pins
SPI: phase, polarity, clock speed, bits, sck/mosi/miso pins
UART: baudrate, bits, stop bit, parity, tx/rx pins
accelerometer: set rate, range (see Add acceleromter range and rate acccessors (in progress). #266)
compass: set sample rate
radio: set freq, channel, address, etc (see Implement simple radio module to send/recv strings. #283)

It might make sense for some of these parameters to also have a "get", but probably not all of them.

Currently UART and SPI have an init() method that works as follows: the device starts up in a default mode (eg UART is used for the REPL) and calling init() is used to completely re-initialise the device. Any parameters that are passed are used, but any parameters that are not specified will take their default values.

One way to proceed is provide a generic config() method that takes keyword arguments to set a value, eg i2c.config(freq=100000); accel.config(range=8). You could get the value using i2c.config('freq').

Another way is to provide an individual method for each value to set (and perhaps get), eg i2c.set_freq(100000); accel.set_range(8).

carlosperate · 2016-06-10T19:57:29Z

@dpgeorge I complete agree with that we should have a proper discussion to unify these configuration APIs, specially now that have a few pull requests that would be affected. However as this touches quite a few modules, it should go into a new issue and then have each PR linked to that. So I hope you don't mind that I have opened #306 for this.

dpgeorge · 2018-03-07T11:32:57Z

i2c.init() was added a while ago in e88e3b2, and it can be used to set the I2C frequency.

Remove MicroBitI2C.get_frequency().

399a763

carlosperate reviewed Jun 3, 2016
View reviewed changes

Remove an unnecessary comma.

c7b1796

MicroBitI2C.set_frequency() -> .init(frequency=)

4ad025f

dpgeorge mentioned this pull request Jun 3, 2016

Add acceleromter range and rate acccessors (in progress). #266

Closed

carlosperate reviewed Jun 3, 2016
View reviewed changes

carlosperate mentioned this pull request Jun 10, 2016

Setting parameters consistency #306

Open

dpgeorge closed this Mar 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for MicroBitI2C.set/get_frequency(). #296

Add support for MicroBitI2C.set/get_frequency(). #296

komeP commented Jun 3, 2016 •

edited

Loading

carlosperate commented Jun 3, 2016 •

edited

Loading

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016

carlosperate commented Jun 3, 2016 •

edited

Loading

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016

carlosperate commented Jun 3, 2016 •

edited

Loading

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016 •

edited

Loading

carlosperate Jun 3, 2016

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016 •

edited

Loading

dpgeorge commented Jun 3, 2016

dpgeorge commented Jun 3, 2016

komeP commented Jun 3, 2016

carlosperate Jun 3, 2016 •

edited

Loading

whaleygeek commented Jun 4, 2016

carlosperate commented Jun 4, 2016

carlosperate commented Jun 4, 2016

komeP commented Jun 4, 2016 •

edited

Loading

deshipu commented Jun 4, 2016

deshipu commented Jun 4, 2016

whaleygeek commented Jun 4, 2016

dpgeorge commented Jun 5, 2016

carlosperate commented Jun 6, 2016

dpgeorge commented Jun 6, 2016

carlosperate commented Jun 10, 2016

dpgeorge commented Mar 7, 2018

Add support for MicroBitI2C.set/get_frequency(). #296

Add support for MicroBitI2C.set/get_frequency(). #296

Conversation

komeP commented Jun 3, 2016 • edited Loading

carlosperate commented Jun 3, 2016 • edited Loading

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016

carlosperate commented Jun 3, 2016 • edited Loading

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016

carlosperate commented Jun 3, 2016 • edited Loading

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016 • edited Loading

carlosperate Jun 3, 2016

Choose a reason for hiding this comment

komeP commented Jun 3, 2016

carlosperate commented Jun 3, 2016 • edited Loading

dpgeorge commented Jun 3, 2016

dpgeorge commented Jun 3, 2016

komeP commented Jun 3, 2016

carlosperate Jun 3, 2016 • edited Loading

Choose a reason for hiding this comment

whaleygeek commented Jun 4, 2016

carlosperate commented Jun 4, 2016

carlosperate commented Jun 4, 2016

komeP commented Jun 4, 2016 • edited Loading

deshipu commented Jun 4, 2016

deshipu commented Jun 4, 2016

whaleygeek commented Jun 4, 2016

dpgeorge commented Jun 5, 2016

carlosperate commented Jun 6, 2016

dpgeorge commented Jun 6, 2016

carlosperate commented Jun 10, 2016

dpgeorge commented Mar 7, 2018

komeP commented Jun 3, 2016 •

edited

Loading

carlosperate commented Jun 3, 2016 •

edited

Loading

carlosperate commented Jun 3, 2016 •

edited

Loading

carlosperate commented Jun 3, 2016 •

edited

Loading

carlosperate commented Jun 3, 2016 •

edited

Loading

carlosperate commented Jun 3, 2016 •

edited

Loading

carlosperate Jun 3, 2016 •

edited

Loading

komeP commented Jun 4, 2016 •

edited

Loading