drivers icon indicating copy to clipboard operation
drivers copied to clipboard

all: do not use machine.TWI_FREQ_* constants

Open aykevl opened this issue 3 years ago • 4 comments

These constants were always a bit weird:

  • they use an uncommon name (TWI instead of I2C)
  • they don't follow Go naming conventions (CamelCase)
  • the constants don't provide much value: their name is their value (100KHZ => 100e3).

Therefore, I propose we remove them and replace them with regular numeric constants. Also see https://github.com/tinygo-org/tinygo/pull/1617.

aykevl avatar Feb 06 '21 10:02 aykevl

I'm a bit divided by this, but generally in favor of this chagne. TWI is clearly confusing but I'd like to have some constants. From a beginner's point of view, it might be difficult to know which value put there, it's easier if there's a list (constants) to choose from. Official names doesn't look good to me either (high speed, fast mode plus, ultra fast mode,...) and then, there's nrf boards with 250kHz.

conejoninja avatar Feb 06 '21 10:02 conejoninja

If you don't know, you can always leave the field zero. It may be slower than necessary but will certainly work.

From a beginner's point of view, it might be difficult to know which value put there, it's easier if there's a list (constants) to choose from.

I think documentation can go a long way here. Something like "if you don't know, leave the field empty. If you want higher speeds, use the value 400e3 which means 400kHz". I plan on doing that in https://github.com/tinygo-org/tinygo-site/pull/142 (which was the direct trigger for this PR: cleaning up the API a bit).

Alternatively, we could perhaps add some constants (to the machine package, or elsewhere) with common frequencies? Something like:

const (
    Freq100kHz = 100e3
    Freq400kHz = 400e3
    Freq800kHz = 800e3
    Freq1MHz   = 1e6
    Freq2MHz   = 2e6
)

...but I'm not entirely convinced this is an improvement.

Also note that you need to know where to find the constants and how they're named before you can use them. Therefore, documentation is essential.

aykevl avatar Feb 06 '21 11:02 aykevl

We could create a named type for it like type I2CFrequency int32 so that the valid values can be found.

This will still allow using simple constant numbers as @aykevl prefers, as well as autocompletion etc. for those who want some human-readable sugar.

Any thoughts?

deadprogram avatar Feb 07 '21 10:02 deadprogram

I’m a proponent of this tack, Ron. Surely a list of “valid” values is what we’re looking for here and this, like you say, provides an extra sprinkling of utility. Amps on 11.

On Sun, Feb 7, 2021 at 05:03, Ron Evans [email protected] wrote:

We could create a named type for it like type I2CFrequency int32 so that the valid values can be found.

This will still allow using simple constant numbers as @aykevl prefers, as well as autocompletion etc. for those who want some human-readable sugar.

Any thoughts?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

loraxipam avatar Feb 07 '21 13:02 loraxipam