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

I2C Lidar Sensor module (VL6180X) does’t work reliably in Attiny85 at 1Mhz #738

Open
Rimbaldo opened this issue Jan 7, 2023 · 10 comments

Comments

@Rimbaldo
Copy link

Rimbaldo commented Jan 7, 2023

I’m doing a project using a bare Attiny85 and an I2C Time of flight sensor (VL6180x).

The module has 10k pullups on the SDA and SCL lines, but I also added 10k resistors externally on the SDA and SCL pins.

I put a 0.1uF ceramic cap between VCC and GND of the Attiny85, running at 5V.

When I use a Digispark at 16.5Mhz, the sensor works fine. When I use the bare Attiny at 8Mhz internal, it also works normally.

But when I use the bare Attiny at 1Mhz, the TOF sensor locks a couple of seconds after running the sketch. The Attiny keeps working normally, but the sensor stops..

Is this “bug” related to the I2C implementation on the Attiny85? Is there any I2C configuration that I could try to make it work at 1Mhz?

@hmeijdam
Copy link
Contributor

hmeijdam commented Jan 7, 2023

I suspect that 1MHz is too slow for the AT85 to keep up with the I2C timing requirements. Keep in mind it has no hardware I2C implementation, but needs to everything in software alongside what you ask it to do else.
You are not using Tinywire or any other external library for I2C? Just the core's Wire.h

@Rimbaldo
Copy link
Author

Rimbaldo commented Jan 7, 2023

@hmeijdam I’m using just the core Wire.h…. Should I try another I2C external library?

I read in Hackaday below that someone changed the I2C stretch clock from 230uS to 1500us and made it work, but when using an esp266 as an I2C master and the Attiny85 at 1Mhz as a slave. Would
it work for me? How could I change this stretch clock time to try this out?

@hmeijdam
Copy link
Contributor

hmeijdam commented Jan 7, 2023

I have no experience to comment on that. Why is the 1 MHz clock speed of importance to you?

Btw, the github version of Attinycore supports more clock speeds between 1 and 8. Maybe worth experimenting with that.

image

@SpenceKonde
Copy link
Owner

I would say no, you should not use an external I2C library. I mean hell, our Wire.h is one of the popular USI-as-I2C libraries (I don't remember off hand which one) - the main difference is that unlike the others, which do not implement a compatible API and hence are not code compatible (the differences were generally really silly too, not like "they left stuff out because supporting it was hard" but rather "they just chose to implement a somewhat different API"). The point of the Unified Wire was to gather the three different I2C implementations that were needed to support I2C on all ATTinyCore parts (a couple have a real TWI, most have a USI, two have neither a USI nor a complete hardware TWI (the 841 and 441 have a slave only hardware TWI), and a third, the 828 has a USI but can't use it for I2C because of a silicon bug - so those three parts have to use a fully software implementation. All of those libraries had non-identical APIs, particularly the software one), and put them all into one library, call it Wire.h, and make sure that the name of the object they provided was "Wire", and that the name of the class was always "TwoWire" like the official library: many libraries expect you to pass it a pointer or reference to an instance of a class named TwoWire, and expects that whatever you pass it will present the normal API. Other libraries don't even bother with that - they include Wire.h, and expect it to provide something named Wire with the normal API.

Our Wire.h uses the preprocessor and knowledge of which AVRs support what to select one of three implementations of I2C and is meant to be a drop-in replacement for wire and maximize code compatibility with other code written for other AVR devices.

But i'm going to echo the sentiment that 1 MHz may just be too bloody slow for some I2C devices. The clock timing is handled in software, and it ends up timing the high and low of SCL by a compile-time calculated number of iterations of the standard three clock timing loop, but at such low clock speeds, you start to see integer-math truncation and rounding effects. One side of the clock ends up with 2 passes through the loop (trying for as 5 us delay, getting 6us) the other tries for 4us, but gets 1 pass through the loop, 3us... Now account for the fact that each clock cycle between these is 1us, not 1/8th or 1/16th of a us, if you look at sending a byte, the SCL high positive edge happens 6 clocks after entering the USI data loop, and is done on the 7th clock, then 2 or 5 clocks in the while loop to see if the pin has gone high, then 3us with the second delay. then after 12 or 15 clocks we get to the end of the high, set by the 13th or 16th clock, which has lasted either 6 or 9 us. Now the low begins, the test is conducted, and we're back at the start. So the entire cycle is at best 16 or 19us of which 10us is low.... so you;re operating well below standard mode speeds. but it's not obviously pathological that I would expect most devices to object. The high is at least regulation length, the low is twice regulation length. It's running at 10/16ths or 10/19ths of the clock speed it's supposed to be, so it's wrong, but only a little wrong, so what? Most I2C devices will run even when SCL is much slower than that, provided the highs and lows are long enough. But occasionally one comes across particularly troublesome "I2C" devices that have problems at certain SCL speeds. The Melexis I2C IR sensors for example are notorious for that (in fact, trying to make those things work resulted in two people independently rewriting the clock configuration code in the mTC/DxC Wire, one of whom went on to reimplement most of the entire library, greatly reducing it's flash footprint and forming a more coherent basis for a the handful of methods that extend the Wire.h API.

A scope trace of the SDA and SCL lines while it works and once it has started failing would be needed in order for me to give any further advice; the source code would also be helpful to rule out other possible problems.

I'm not sure what is meant by changing the clock stretching. Clock stretching is something that the slave device may do: After receiving the 8 bytes of data, the slave device is permitted to hold the clock line low while it processes that byte, before deciding to reply with an ACK condition or a NAK condition. The Arduino Wire library, like most Arduino libraries was not, shall we say, designed by embedded design experts with a close eye on performance (afaict, most arduino libraries, particularly official ones, were neither designed by experts, nor with particular concern for performance). Maybe they meant lengthening the time the ESP would wait while the clock was stretched? That would make sense when trying to talk to an ATTiny85 at 1 MHz acting as an I2C slave. The AVR architecture, the limitations of C/C++, avr-gcc's questionable compiling decisions (questionable in terms of performance, not correctness), the Arduino paradigm and the API dictated by that, and many of the more boneheaded idioms common in the Arduino community - they all contribute to most cases of surprisingly poor performance, but all of these issues are at a maximum here. First - There are two interrupts associated with I2C as slave, and the way this maps to the ArduinoAPI are:

  1. Start Condition interrupt; Generally just used to wake from sleep mode on a start condition. Probably the best use of it is an empty interrupt. I don't think Wire.h uses it at all
  2. Something else, which you determine by reading status registers either:
    a. This is the buffer squawking "feed me! feed me!" during a read "Finished sending that byte to the master, quick where's the next one?"
    b. "You've been addressed for read, perform any necessary steps",
    c. "you're being addressed for write, get ready",
    d. "you just received a byte"

(with real harware I2C it's similar in the big picture view as well, though ofc there are some differences. At least on modern AVRs you can get an interrupt on the stop too.

The two callbacks that you get in arduino are in those onRequest and onReceive handlers. OnReceive is called at 2d (or shortly after if there's an interrupt for stop), and onRequest at 2b. As noted, 1 isn't used (it may be on the newer cores, with a very short ISR), so 2a, 2c, and most cases of 2d are handled automatically. Meaning that you don't just need to set up the first character in onRequest, you have to set up ALL OF THEM. And you are in an ISR where you are rudely standing on top of the SCL line holding it to ground, then finally when that returns, it releases scl and the master clocks out the byte or bytes (you don't know how many he wants - that is signalled by the master nacking instead of acking the final byte.) Thus, the clock stretching is heavily front-loaded immediately after the address match. most Arduino users are not very cognizant of the considerations of code that is running in an ISR (though just by calling a function pointer, they've ensured that it pushes and pops things as if it were using 16 working registers, making the impact of habitual poor practices less dramatic, and are often not particularly good programmers at all and/or don't have much sense for what is slower or faster than what, and even though the may only be one pointer the whole time, even with LTO it isn't allowed to just inline it even (which would reduce the minimum time spent pushing and popping to under 20 clocks - though that's only the minimum. The more memory the ISR needs for variables it works with (local or otherwise) the longer that time is (3 clocks per byte of maximum register use), and sometimes there are slow things that you don't realize are slow that you might do then. And you are generating the maximum amount of information the master might want. The master might only be reading 1 byte, or he might be reading 32, and you don't know until you're told the counter overflowed, and check status and see that there's a stop condition, they're done. In effect, the sum of the time it takes to generate every byte in the buffer will be taken when executing onRequest() address match interrupt.... This is basically everything you don't want all at once: You've got user code that has to do a somewhat large operation in an ISR which they will likely not do efficiently, plus the ISR has a bunch of additional under-the-hood logic, plus the worst case scenario is realized every time in terms of how much must be prepared, and it calls functions from an ISR.

DURING ALL OF THESE INTERRUPTS, THE CLOCK IS BEING STRETCHED BY THE SLAVE ARDUINO!
They do all the work of preparing for any size read for every read, and this is done in an ISR by user code - and since Arduinio folks often don't realize just how much faster or slower certain things are, they don't always write that interrupt with performance as a priority, don't realize that they need to and wouldn't know how to if they did.
So yeah I can imagine that taking long enough that they'd have had to change the clock stretch timeout I suppose.

(yasee one could also imagine a scheme under which feedme and read address match would call a "give me next byte" routine. But AVR-GCC and avrlibc and the ABI smack that down, because even if your getNextByte() is simple, if it gets passed as a pointer, it gets icalled, and if it gets called the calling conventions apply, and thus the 14 extra call-used registers, of which you may barely use any, get pushed and popped by that interrupt calling it once per byte instead of once per transfer (if most transfers are 1-2 bytes, but the larger the reads, the worse the performance, and it becomes significantly less efficient for large reads) Neither path is performant. Now imagine that a buffer and a pointer in that buffer were maintained, the isr was naked, prologue and epilogue hand written, onrequest would push x and any other register, load the pointer into x, ld X+, store incremented pointer, then using one of those registers that you can ldi into as scratch space, write to the status and control registers for USI or TWI as appropriate, store in some global flag somewhere that the master ended the read and return either way. If that were all the interupt did, which it isn't, it would be like push r26; push r27; push r24; in SREG r24; push r24; lds ptrlow, r26; lds ptrhigh, r27; ld X+, r24; out r24, usidr; in usicr, r24; andi 1<<USIPF; sbic r24, USIPF; sbi GPIOR0, 0; sts r26, ptrlow; sts r27, ptrhigh; pop r24 out r24, SREG; pop r24; pop r27; pop r26; reti; just under 40 clocks I think. then during normal operation, the buffer would be periodically updated (with those actual writes guarded by disabling interrupts for the few clocks it took to write them). Meanwhile the first byte written would set the buffer pointer, and subsequent bytes would start writing to the buffer, subject to a map of which parts of the buffer were read only from the master's perspective (like the data registers on a sensor). Wire would be far more efficient that way than the way they did it, or the naive "give me the next byte" type implementation. But of course, it is a completely and totally different model than the one that Arduino chose, and even the cleverest user code can't emulate this bog standard interface using the Wire API because you don't know how many bytes the master read, or whether they're done reading (no api calls for that provided) That's why you see the contrast that you do between real I2C devices with a sleek intentional-feeling consistent interface, while Arduinio I2C slave devices all use I2C like it was a glorified (errr... maybe that isn't the right word) serial port with a clock line. The API doesn't expose the information needed to produce the de facto standard behavior a normal engineer would expect in an I2C device - it's almost impossible to make an I2C slave that wouldn't get laughed at by any professional embedded developer with the Wire library.... (On mTC and DxC, the original implementation of the library was so terrible (worse than any implementation here, despite the fact that the hardware is far more helpful over there, being a proper master slave TWI, not this Useless Serial Interface. All you really get from the USI is the automatic clock stretch and schemes for clocking out data that have very little advantage ov) that it was entirely rewritten almost entirely de novo. We still of course followed the Arduino API since we have to maintain compatibility with existing libraries, but me and the collaborator on this both added a couple of methods which fill in the gaps in the API and expose new hardware featrures (a slew of small simple ones, and one really huge complicated one (which I wouldn't have touched with a ten foot pole), but our API is a superset of the standard one, and I made sure that one could at least emulate a normal register model I2C device, albeit one that stretched the clock unusually long when receiving a read address match, and added methods to access a few new features (1 huge complicated one, and several trivial ones) that the mTC and DxC hardware supports)

But this doesn't have direct bearing on your problem, because you're not making the ATTiny act as a wire slave.

(can you tell that I've spent way too much time thinking about problems with Wire? Mostly over on mTC and DxC recently.)

*the PLL, when enabled either by selecting PLL as clock source, or by writing the appropriate register) multiplies the internal oscillator speed by 4 - that 64 MHz clock can be used by the Timer1 and/or divided by 4 to generate a 16 MHz system clock; the timer1 here is is not a normal Timer1. Unique to the x5's, it's an "8-bit high speed timer" instead of the usual 16-bit ones that almost every classic AVR (the x61's have a somewhat similar 10-bit high speed timer with an extra channel, and like the x5, each output has an optional complementary output pin with a dead time generator. Both of these parts are designed with motor control applications in mind, where you often need a higher pwm frequency, and you need programmable deadtime to prevent shoot-through that would burn out the H-bridge. The x5 works for 2-pole BLDC motors, while the x61 can do 2-pole or 3-pole ones The wacky timer is great if you want to drive a BLDC motor - everyone else would probably be much happier had it been a normal 16-bit timer.

@SpenceKonde
Copy link
Owner

I will note that 2.0.0 will have 2 MHz and 4 MHz via internal oscillator available as a clock speed option on all parts (starts up with clock prescale set to 8 by fuse in case voltage is too low for 8, then shifts prescaler to /4 or /2 as appropriate), providing intermediate options between 1 and 8.

Also, if you're running at 1 MHz for power consumption, be aware that you save much more power when in sleep mode than when active, even at 1 MHz. 1 MHz is still >1mA, power down sleep is under 1 uA. And the chip draws more current at 2 MHz - but not twice as much. And with the timing you end up with at 1 MHz, you're already prolonging I2C communication cycle significantly. Assuming you spend most of your time in power down sleep, and are just periodically waking up to take those measurements, if you spend 1% of your time awake at 1 MHz the average power consumption is higher than if you spend 0.5% of the time awake at 2 MHz (whether or not you account for the power consumption during power down sleep, or simply approximate it as zero. With wake cycles of around 1% zero is a good approximation of the contribution of the current in power down sleep mode assuming it is being used correctly and is reaching the expected current. Another order of magnitude, though and you start noticing it, and roughly 2 orders of magnitude lower wake fraction (ie, 0.01% awake) is around where twake*Iwake is of the same magnitude as tsleep*Isleep, marking the boundary between the regime where power consumption is dominated be the power used during the brief waking periods, and the regime where the power is dominated by the consumption while sleeping.

Remember that when you are sleeping, the clock is stopped so it doesn't matter what the nominal system clock is as far as the power consumed during sleep mode (what matters there is that you remembered to disable the ADC and that you don't have any floating pins, and that you don't have any loads connected to any outputs (obviously), and whether you have the WDT enabled (for waking up on a time interval) or BOD enabled (to hold the part in reset if the supply voltage is below the specified threshold. Note that while in BOD, the power consumption is markedly higher than power down sleep. That is to say, no, you cannot use BOD alone as UVLO to prevent damage to lithium batteries from overdischarge (BOD reset state draws orders of magnitude more current than power down sleep.

It's objective is to prevent the processor from misbehaving, as AVRs are wont to do when their safe opperating conditetions are exceeded sufficiently egregiously (in terms of dF/VDD, VDD and . (for example, calculating that 2+2 = 0 (non-zero results of mathematical operators coming out as zero, and less often, mathematical results coming out with bits flipped seemingly at random. This directly (incorrect status bits resulting from an instruction, or indirectly (bad values used in calculation) corrupts the SREG. The corruption of SREG, or simply incorrect execution of flow control instructions thus deranges program flow. Often the final moments involve execution bounding across the flash, with each call encountered stacking a possibly corrupt pair of values onto the stack and each return jumps to whatever the top of the stack contains a pointer to, which may have been corrupted before it was calculated, or when it is read, and this further deranges program flow.; usually the madness quickly reaches a point when the value at the top of the stack is 0x0000 (remember how non-zero results coming out as 0 is the most common failure mode), and execution hits a return statement, at which point execution ends up at 0x0000, and no reset cause has been set because no reset occurred. At this point, as long as the system manages to successfully evaluate the less than a dozen instructions in the path from 0x0000 to when we fire software reset because re realize the reset was dirty, it will reset cleanly. Assuming the overclocking was generated internally, this should bring part back in spec, until the sketch tries to make it perform beyond abilities again. BOR sets a voltage tjreshold if the voltage may suffer brownouts to keep the chip idle, It is unconcerned with the battery, which should have it's own UVLO to prevent overdischarge!!

when you're in power down sleep mode, there is no system clock being generated. Power used while in power down sleep doesn't change regardless of the system clock configuration is the same whether the clock is set to 16 MHz PLL clock or 128kHz WDT clock,

@Rimbaldo
Copy link
Author

Rimbaldo commented Jan 9, 2023

Thanks for all your deep explanations!!

I tested in 4Mhz and the I2C sensor worked normally!

I'll scope the I2C lines later at 1Mhz to see what happens and I''ll get back later to you with the data... Exactly how and what should I measure in the lines?

Indeed my goal is to save current, that's why I wanted to use 1Mhz. I didn't think in using power saving modes (sleep modes) because I have this Attiny with the Lidar sensor connected through a 1 meter cable to another Attiny167 at the lipo Battery side. And they both exchange data through a 1 wire transfer half-duplex protocol. So they have always to be in sync with each other.

But the sensor draws more current than the Attiny... Currently at 4Mhz I'm drawing 5mA at the Attiny and more 5-10mA at the sensor...

@SpenceKonde
Copy link
Owner

SpenceKonde commented Jan 20, 2023

They should do the same thing at 1 MHz that they do at 4. Except that they will not do it quite with the same timing; it may be obvious what it's doing wrong (i, if a high or low is coming out way short), or it may be harder to see what it;s doing wrong. but I predict that you'll see when you're doing something it doesn't like, either it won't respond at all, or it will respond by holding one of the lines (likely the clock line) low indefinitely. That's what a lot of things do when you do something on the bus that confuses them

Edit: Oh - I just checked - the lidar sensor also has power down modes, you can put it to sleep when it's not in use too (and would probably put both ATtiny parts to sleep in this case too...)
I think the half-duplex serial line could also be used as a way to wake from sleep. Put a PCINT on it that wakes the chip, and when you think the other chip is asleep, just send a null, and have the tiny that's told when to sleep and wake ignore single characters received if the chip was previously sleeping. For waking a tiny167 with it's hardware serial port, there may be a way to get it to wake and successfully receive the data if the baud rate is low enough. You actually can do that on modern AVRs (though with caveats due to errata in that feature; if the old USART can do a wake on start of frame, it actually works like it says in the datasheet. On the new ones, it uh doesn't quite match the datasheet (you must turn off SFD (start of frame detection) before doing the normal serial read stuff - my cores actually do that now on the new parts, as of about when the first parts that didn't have that bug were released (ie, last year. Modern AVRs started shipping in 2016)). I don't remember if the classic AVRs could, and if they only could in theory, or whether it worked well). Regardless the PCINT method (or a pin interrupt on a modern AVR) could certainly be used to wake either part and you could have it just ignore the first character it sees after waking, and as long as both devices know at what times the other device is possibly sleeping, and hence to preceed a message with a null (0x00) followed by a short delay to let the other device fully sort itself out, you could have them sleeping and wake eachother up that way. But you'd also need to put the laser ranger thing to sleep (I think that's an I2C command - or it may be an I/O pin that puts it into sleep mode. Refer to the datasheet (or pay me go study it, I guess) for more info. But there's definitely a low power mode on that sensor, but while actively ranging, it definitely sucks power.

I mean, of course it does, it's periodically firing a laser, timing how long it takes before it sees the reflection, and recording that time. Light moves pretty fast, so that's a fairly fast sensor it's gotta have. Making the sensor fast is gonna make it use more power (though it sounds like you've got it running full tilt, the active power they quote is only 1.4mA - making measurements at a fairly short distance at 10 Hz sampling frequency.

@SpenceKonde
Copy link
Owner

2.0.0 will also have a 2 MHz option, which you may find works.

if you don't care about millis, obviously, you just manually set the prescaler to 2 when talking to the lidar and back down to 8 when not doing so.

Do note, for all AVRs, if you are CPU-bound for the one task you do while away, ie, running at speed X takes 10 seconds, and running at speed 2X takes 5 seconds - and you spend the rest of the time in powerdown sleep, you are always better running at speed 2X, because Idd(X) < Idd(2X), pretty much over the whole AVR product line. I know of no exceptions. Running at half the speed takes more than half the current (this is how it has been on every AVR I've looked into power consumption for, largely to smack down people who kept asking me for lower clock speeds to save power, and then you look at their code and they're spending all their time in power down sleep when the clock isn't even running and the clock speed doesn't matter at all and so what they were asking for was a mode that would have problems for certain due to the low speed, and would actually use MORE POWER than what they were doing now! \o/

@SpenceKonde
Copy link
Owner

SpenceKonde commented May 1, 2023

I'll scope the I2C lines later at 1Mhz to see what happens and I''ll get back later to you with the data... Exactly how and what should I measure in the lines?

If you've got a digital scope, the most useful thing would be a single trigger showing the successful transfer (with the horizontal scale adjusted so it just fits on the screen), and then at 1 MHz, you should see that something isn't coming back right. My ideal thing to compare would the the 4 MHz working one and the 1 MHz non-working one with the scope's scale adjusted so that a byte took up about the same amount of space on the screen for both traces, or with the same scale but, which is such that the slower of the two devices would take up the full screen, were it to work, which as we know it doesn't - I'm very interested to see what's going on too (and if 2.0.0 works, you can test that version too (I would be extremely interested to hear if it was busted under conditions that used to work - i have a suspect line in the USI portion of wire.cpp (I've actually got two versions of the file, differing in that line, and I don't know which direction the fix was in) and get a trace at 2 MHz and tighten the noose around the precise point at which the bug manifests. We'd know to within a power of two what the minimum clock speed was instead of just knowing that it was between 1 and 4, but we'd also know what the key SCL clock that it was actually achieving was that did and didnt work.

@Rimbaldo
Copy link
Author

Rimbaldo commented Sep 20, 2023

Hi! Just an update... After updating to the latest 2.0 core version of three weeks ago, my VL6180X I2C Lidar sensor module is working normally in my Attiny85 at 1Mhz! In the previous core I had installed, if I got from 8Mhz to 4 and to 1Mhz, the speed (even time in seconds from millis() variables) would get slower and slower to the point of the sensor being unusable. Now at 1Mhz it works normally as in 8Mhz, with the timings also correct.

One weird thing I note, and I don't know if it's an Arduino IDE 2.2.1 or an Attiny 2.00 Core bug, is:

When I re-save the sketch I'm working with another name (a backup, for instace), most times I have to re-burn the bootloader at the desired speed, and then re-upload the sketch, even if the bootloader was burned at the desired speed beforehand. but after that it works normally. It seems that when I save some sketch with another name, it resets the speed internally in the IDE, not in the chip, to the default (8Mhz in both my Attiny's 85 and 167)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants