ONEWIRE_PRESENCE_TIME_MIN issue

[joysfera]

Even though the ONEWIRE_PRESENCE_TIME_MIN is set to 160_us (and the comment right next to it says "was 125 us" to confuse reader a bit) it actually generates pulse more than 272 us long (which is out of spec: 60 to 240 us). This is on Pro Mini ATMEGA328P @ 16.000000 MHz.

It seems like the library's wait() function is not working correctly. When testing in a separate sketch both wait(160) and wait(160_us) take more than 230 us.

Other timing/delay functions of this library seem to work OKish (waitLoops1ms() takes 892 us).

Since the wait() is used just in the PRESENCE pulse generating code, the rest of the library seems to work just fine.

So it seems safe to me to lower the ONEWIRE_PRESENCE_TIME_MIN from 160_us to 60_us that makes it about 104 us in real time, very much like original DS18B20 and mainly within the specs.

BTW, original DS18B20 can cope with the extended PRESENCE pulse of this library but the Chinese fake/counterfeit clones that started appearing in 2018 do not have proper timing and fail in a scary way. Unfortunately noone but me yet has documented that Chinese "geniuses" copied the Dallas/Maxim DS18B20 and started spreading the buggy copies around the world.

[kutukvpavel]

I rewrote this a bunch of times already and I am a bit confused now.
Anyway, here are my initial thoughts:

Well, I dug out my old configs and notes, and remembered what's going on.
If you substitute wait(PRESENCE_ ... [od_mode]) with standard waitLoopsWhilePinIs(ONEWIRE_TIME_PRESENCE_MIN[od_mode], false), you'll see that your presence pulse is now shorter than it should be, but in perfect consistency with calibration's "1mS". I.e. I am calibrated to 851uS as "1mS", my presence will be 0.85*160=136uS. You have a little different calibration value, probably due to cheap chinese crystals/caps being not so great too (btw, have you measured oscillator frequency to claim 16.000000MHz with 1Hz precision?). That's why wait() exists, because in this particular situation a little comment in platforms.h comes into play:
constexpr uint8_t VALUE_IPL { 13 }; // instructions per loop, compare 0 takes 11, compare 1 takes 13 cycles
It is an inherent issue of keeping time in cycles per some code execution: when using an optimising compiler (or when some hardware optimisations take place) code execution speed may depend on actual values being processed. So some pieces of code actually need VALUE_IPL of 11 (including showPresence, running modified line I mentioned above with 11 IPL gives right timing), some of 13 (probably read-write bit fns, where pin state can vary and cumulative error due to looping is small). During running "advanced calibration loop" I came across ~925uS pulses, which represent the latter.
For the sake of simplicity of coding and calibration for different architectures this aspect is neglected. Also such timekeeping is required only for AVR, STM32's for example have a hardware timekeeper, ESP probably too. Proper implementation of timekeeping for all arduino architectures would require major changes of lib's guts. Probably it's easier just to introduce some tuning coefficients for architectures.
Sorry for disinformation about out-of-the-box precision of my own presence pulse :)
Somehow I remembered that I've achieved good precision with respect to the standard, but not how I achieved it.

After that I started to think how tunings can be implemented. Also I wanted to figure out which config parameters correspond to which real IPL value. For it I decided to rewrite calibration sketch a bit and expose waitLoopsWhilePinIs() into it. That required moving function definition into header file (as I was told by linker error, sigh). At this stage I noticed that relatively new arduino IDE's enable Link Time Optimisation (-flto key). And then it hit me! LTO is a very powerful post-compilation optimisation tool, with its help I managed to pack atmega16's old program into atmega8 not so long ago! And i believe it became enabled by default after this lib was initially written. Even though I did not have a chance yet to study LTO vs non-LTO generated assembly codes, it's highly likely it had some kind of impact. Mainly because after moving function into header this sketch

void loop()
{
    delay(5);
    pinMode(pin_onewire, INPUT);
    digitalWrite(pin_onewire, LOW);
    hub.showPresence();
    delay(1);
    // advanced calibration loop --> try to track and measure it with a logic analyzer
    if (true)
    {
        io_reg_t debug_bitMask = PIN_TO_BITMASK(pin_onewire);
        volatile io_reg_t *debug_baseReg = PIN_TO_BASEREG(pin_onewire);
        const timeOW_t loops_1ms = timeUsToLoops(uint16_t(VALUE1k));
        timeOW_t loops_left = 1;
        while (loops_left)
        {
            DIRECT_MODE_INPUT(debug_baseReg, debug_bitMask); // Fast high low flank
            DIRECT_MODE_OUTPUT(debug_baseReg, debug_bitMask);
            hub.waitLoopsWhilePinIs(loops_1ms, false);
            DIRECT_MODE_INPUT(debug_baseReg, debug_bitMask); // switch to HIGH
            loops_left = hub.waitLoopsWhilePinIs(loops_1ms, true);
            DIRECT_MODE_OUTPUT(debug_baseReg, debug_bitMask);
        }
        loops_left = 1;
        while (loops_left)
        {
            DIRECT_MODE_INPUT(debug_baseReg, debug_bitMask); // switch to HIGH
            hub.waitLoopsWhilePinIs(loops_1ms, true);
            DIRECT_MODE_OUTPUT(debug_baseReg, debug_bitMask);
            loops_left = hub.waitLoopsWhilePinIs(loops_1ms, false);
            DIRECT_MODE_INPUT(debug_baseReg, debug_bitMask); // Fast high low flank
            DIRECT_MODE_OUTPUT(debug_baseReg, debug_bitMask);
        }
    }
}

gives precise showPresence timing and precise 1mS!
If only I did not mess something up again while editing config parameters back and forth...

P.S. All required functions are moved to public section in OneWireHub.h

[joysfera]

@kutukvpavel I'm not sure what's the point. I reported that wait() is imprecise (by 75 %) when built with Arduino IDE 1.8.5. I am aware that it could be due to a compiler optimization. I suggested shortening the config parameter in order to get the real presence pulse length within the specs.

You say that if you replace wait() by some other function it works better. Fair enough. It's up to the library maintainer to choose whether to fix wait() or to replace it with something else.

I am most happy that I managed to debug the communication issues with Chinese fake "waterproof" DS18B20. That was the most challenging piece.

FYI, LTO is not enabled by default because it causes serious issues, AFAIK.

P.S. BTW, I suspect the wait() is so off because of the volatile keyword used intentionally here and there. That might indeed cause wild differences when different GCC versions are used.

[joysfera]

@kutukvpavel as for the calibration - you measured 851 us, I measured 853 us, so it's perfectly even - even though the function should have been waiting for 1000 us :-)

[kutukvpavel]

The point was I thought I've found the cause of the bug and a clean solution. Maintainer pretty much abandoned this lib, last commit was 2 years ago, he responds to issues in comments once in about 6 month and that's all. So there is almost no hope anything will get fixed "on its own". Especially because for most of the users it works and issues are rare and complicated.
By now I've studied some more cases of lto/non-lto compilation. It is indeed enabled by default in 1.8.5 (see platform.txt for AVR). The result is puzzling: timings work as expected for this (1000uS):

void loop()
{
    delay(5);
    hub.showPresence();
    <1mS switching stuff here>
}

but are off by 15% for this

void loop()
{
    delay(5);
    <1mS switching stuff here>
}

Contraversely, LTO in this case not breaks, but makes the first variant work. Without LTO both are 850uS.
Of course reducing timings in config file is pretty simple and the original issue you had is now resolved, but now the case is getting interesting. Never came across a bug that can be fixed with LTO. I think I will continue research.

[joysfera]

@kutukvpavel I gave it a second thought and I agree with you that the wait() should be replaced by the timing/delay functions used at all other places as it would be consistent at least (even though less readable because in PRESENCE pulse you actually don't wait for any pin change). And mainly all timings in config file would be off by same proportion which would make tuning easier.

Sorry for the LTO confusion - I have another AVR target installed in the IDE (for ATMEGA328PB support), it comes with newer GCC and the LTO is specifically disabled there because otherwise it caused weird issues.

Anyway, I compile this library with stock AVR target and since I'm getting 850-ish µs it's apparently not LTO-fixed.

BTW, if 1ms is 850 µs then all OneWire timings are off by another 15 %. It would be great to fix the library for AVR to get 1 ms = 1000 µs.

It would perhaps make sense to test/focus on Arduino IDE 1.8.8 with much newer GCC. Things might be quite different there again.

Keep researching. Having a 100% working OneWire slave is always a good thing.