Skip to content

Make sure clock pulses have a defined length #7

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make sure clock pulses have a defined length #7

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a654d3c
Make sure clock pulses have a defined length
jannic Dec 5, 2023
5b75875
Change DIO level in the middle of the (low) clock pulse
jannic Dec 8, 2023
25e5e7e
Revert "Change DIO level in the middle of the (low) clock pulse"
jannic Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ where
return Ok(());
}
self.delay();
self.delay();
}

Err(Error::Ack)
Expand All @@ -117,6 +118,7 @@ where
self.send_bit_and_delay(Bit::ZERO)?;
self.dio.set_high()?;
self.delay();
self.delay();

Ok(())
}
Expand All @@ -128,6 +130,7 @@ where
} else {
self.dio.set_low()?;
}
self.delay();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.clk.set_high()?;
self.delay();

Expand All @@ -141,7 +144,7 @@ where

const MAX_FREQ_KHZ: u16 = 500;
const USECS_IN_MSEC: u16 = 1_000;
const DELAY_USECS: u16 = USECS_IN_MSEC / MAX_FREQ_KHZ;
const DELAY_USECS: u16 = USECS_IN_MSEC.div_ceil(MAX_FREQ_KHZ * 2);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He-he, here it was speedup

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷)


const ADDRESS_AUTO_INCREMENT_1_MODE: u8 = 0x40;

Expand Down