Make sure clock pulses have a defined length

[jannic]

Currently, clock pulses can be arbitrarily short, as the whole clock delay is done after setting clk to low, setting the data bit, and setting clk to high again. On fast processors, the clk pulse can become too short to be recognized reliably, which may be the cause for issue #5.

To fix that, split the delay in two equal halves, and do one of them while the clock is low.

I duplicated all other calls to delay() to keep the delay lengths stable. This might not be necessary, but shouldn't hurt either.

[igelbox]

I do believe this could be fixed outside of the library that's why I force to supply the delay mechanics from the outside.
Also I do believe hal::delay::Delay must work well. Otherwise it's something wrong with HAL not the library itself.
If not, you could implement the fixes as part of custom external delay routine like NoDelay example.

So, for now I'm not sure this PR changes are required for all of the devices this library may work on.

[jannic]

The issue is not that delay doesn't work as expected. But currently, there is no delay at all between self.clk.set_low() and self.clk.set_high(). So the length of the clock pulse only depends on the execution speed of the code. If the CPU is faster, the clock pulse will be shorter. At some point, the pulse will be too short to be detected by the tm1637. Again, as there is no delay call at all in that part of the code, that can't be fixed from the outside by providing a different delay implementation.

I'm not sure about the exact specs of the tm1637, but according do this data sheet, the clock pulse must be at least 400ns long: https://raw.githubusercontent.com/avishorp/TM1637/master/docs/TM1637_V2.4_EN.pdf

The issue came up with an rp2040, which runs at 125MHz, and the code is optimized down to 6 clock cycles (if I counted correctly), making the clock pulse 48ns long. Much shorter than the specified minimum.

I think the safest option is to create a clock signal with 50% duty cycle, and that's exactly what I implemented with the patch.

[igelbox]

Ouch, now I see. Thank you for being patient and outlining the bug.

---

Also, it seems my code for reading ACKs is weird and even dangerous coz it ties the pin to 1 but chip does "lower the DIO pin" in response which may cause a quick short circuit:shit:
Now I think it would be better to somehow make the DIO pin floating just before receiving ACK.
But, since I have no hardware to test on, I'd postpone such a fix ¯_(ツ)_/¯

UPD: ah I used into_open_drain_output in the example, so it seems to make a floating pin after writing 1.

[igelbox]

The datasheet says "the clock frequency should be less than 250K" and now I have no idea why this library works at all =) coz it sends data at 500khz speed.

So I believe we could leave the original value here then 2 delays would result in 500khz / 2 = 250khz clock frequency (like datasheet requires).

[igelbox]

He-he, here it was speedup

[jannic]

https://raw.githubusercontent.com/avishorp/TM1637/master/docs/TM1637_V2.4_EN.pdf specifies the maximum frequency as 500kHz. Perhaps there are different revisions of the chip with different requirements?

[igelbox]

And they mention "the clock frequency should be less than 250K". However it may relate to reading not writing. Not sure though.

I had just one chip. I'm not an experienced developer of embeddeds 🤷)

[igelbox]

Could you please move this before if and test whether it works or not? It would make it more symmetric, e.g.:

• low CLK, delay
• set DIO
• high CLK, delay
• (sometimes in start and stop routines) set DIO

[jannic]

Yes, that sounds like a good idea. The current patch is intentionally doing the minimal change to make the clock pulse longer if necessary.
Do you have a better data sheet than https://raw.githubusercontent.com/avishorp/TM1637/master/docs/TM1637_V2.4_EN.pdf? The timing information in that one confuses me a bit. It doesn't help that the table on the bottom of page 10 lists some minimum timings without exactly telling what they measure.

[jannic]

Wait: No! With that change, now there is no longer a delay between two bits:

• first bit:
  • low CLK
  • delay
  • set DIO
  • delay
  • high CLK
• next bit:
  • low CLK
  • ...

So there's the same problem as initially, only that now the high time between two bits can become arbitrarily short. That doesn't work reliably either.

[jannic]

I also tested this variant, it works but is less reliable:

      fn send_bit_and_delay(&mut self, value: Bit) -> Res<E> {
          self.clk.set_low()?;
          self.delay();
          if let Bit::ONE = value {
              self.dio.set_high()?;
          } else {
              self.dio.set_low()?;
          }
          self.clk.set_high()?;
          self.delay();
  
          Ok(())
      }

In this combination, if I use a push-pull pin for CLK, it does no longer work.

I think the issue with this is that the DIO change happens to shortly before the rising edge of CLK. If the DIO line rises slightly slower than the CLK line, the data line will be sampled before the signal level is valid.
(This could also happen if both CLK and DIO are open-drain outputs, if the rise times are different due to tolerances in the pull-up resistors.)

In summary, this version seems to be the most reliable one, at least without adding additional delay statements:

    fn send_bit_and_delay(&mut self, value: Bit) -> Res<E> {
        self.clk.set_low()?;
        if let Bit::ONE = value {
            self.dio.set_high()?;
        } else {
            self.dio.set_low()?;
        }
        self.delay();
        self.clk.set_high()?;
        self.delay();

        Ok(())
    }

Another option would be to half the individual delays again, and have delays both before and after setting DIO. But that would require delays shorther than 1µs, or would slow down the communication.

[igelbox]

due to tolerances in the pull-up resistors

Darn circuits) Also there may be some filtering capacitors (like mentioned in http://www.microcontroller.it/english/Tutorials/Elettronica/componenti/TM1637.htm) which may make signal fronts less predictable. I think the best/safest solution is shift DIO vs CLK in 90 degree out of phase, e.g.:

• low CLK
• delay 1µs
• set DIO
• delay 1µs
• high CLK
• delay 2µs
  but yeah it'll slowdown the communication to about 250khz

[igelbox]

Do you have a better data sheet than

Nope. Moreover, I cannot recall whether I followed some datasheet while implementing the lib or just looked into some Arduino C++ libs implementations (

[igelbox]

Wait: No! With that change, now there is no longer a delay between two bits:

But lib has delay just after set_high -

tm1637-rs/src/lib.rs

Line 135 in 25e5e7e

self.delay();

In this combination, if I use a push-pull pin for CLK, it does no longer work.

Okay. I have no hardware to test now. So, I'll choose the variant that works for you.

[igelbox]

Does it work with just one additional delay in the send_bit_and_delay method? (I mean without dividing the DELAY_USECS by 2)

[jannic]

Also, it seems my code for reading ACKs is weird and even dangerous coz it ties the pin to 1 but chip does "lower the DIO pin" in response which may cause a quick short circuit:shit: Now I think it would be better to somehow make the DIO pin floating just before receiving ACK. But, since I have no hardware to test on, I'd postpone such a fix ¯_(ツ)_/¯

UPD: ah I used into_open_drain_output in the example, so it seems to make a floating pin after writing 1.

Yes, that's nothing the library can change, as the pin config gets done by the caller. Perhaps add some doc comment explaining that the DIO pin should be configured as open-drain output?

[igelbox]

Yes, that's nothing the library can change, as the pin config gets done by the caller. Perhaps add some doc comment explaining that the DIO pin should be configured as open-drain output?

Well, it appears the library already forces the DIO pin to be open-drain by DIO: InputPin<Error = E> + OutputPin<Error = E> constraint. At least the example doesn't compile with let mut dio = gpiob.pb7.into_push_pull_output(&mut gpiob.crl);:

    = note: the following trait bounds were not satisfied:
            `<PB7<Output<PushPull>> as embedded_hal::digital::v2::InputPin>::Error = _`
            `PB7<Output<PushPull>>: embedded_hal::digital::v2::InputPin`