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

Overhaul the pin handling (not backwards compatible) #224

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

Conversation

matthijskooijman
Copy link
Collaborator

This PR will track changes to the pin handling. The original goal was to allow using the analog pins as digital inputs through bitlash, as well as make the analog pin reference selectable. At the same time, this changes some of the internals of the pin handling for better consistency and cleaner code. This does mean this will become backwards incompatible - function names have changed (both in bitlash and C++) and the pin modes now have different numbering. The pin reports will change as well, but that wasn't implemented yet (neither has analog reference selection).

So this is not finished yet, just wanted to collect some feedback early.

Some more detailed list of changes, from the biggest commit:

  • Split INPUT mode into INPUT_DIGITAL and INPUT_ANALOG. This allows
    using analog pins as digital pins as well. It requires explicitely
    choosing either of these modes, except when using pin.makeinput,
    which autoselects based on the pin type.

  • Rename and renumber the pin mode constants. This makes things a bit
    more consistent and by removing the negative values we allow using
    the upper bits of the mode as flags or other values.

  • Generalize the "pin mode" value into a "pin config" value, which
    contains the pin mode, combined with other values. Right now, this
    is only a flag for wether the pullup is enabled (which was previously
    part of the mode), but in the future flags like "reports enabled" or
    values like the analog reference to use can be included.

    This removes the pin.setmode command and replaces it with pin.config,
    which works similar. It accepts the following modes: disabled,
    disconnected, input_digital, input_analog, output_digital,
    output_pwm. Additionally, the value flag_pullup can be added to
    the input modes to enable pullups (defaulting to disabled). e.g.:

       pin.config("d8", output_digital)
       pin.config("a0", input_digital + flag_pullup)
    
  • When setting a pin to output_digital, it now gets written to LOW by
    default. Previously, the value would be unchanged, which meant that
    if the pullup was previously enabled, it would be HIGH, otherwise it
    would be LOW. When using pin.config, the value can be overridden
    (which is useful if you must go from input-with-pullup to output-high
    without going through output-low for some reason).

  • pin.makeinput no longer takes input or input_pullup as the second
    argument, but instead expects a 0 or 1 value to disable/enable
    pullups. Leaving it out makes the default depend on the pin type (see
    next item).

  • When setting an analog pin to input using pin.makeinput, it no longer
    defaults to having the pullup enabled. Since the pullup is connected
    to 3.3V, that's too high for the ADC, and if the voltage sensed comes
    from a voltage divider, having the pullup enabled will even influence
    the values read.

  • PinConfig and Mode values are now wrapped in structs, so you can do
    things like config.mode().input() to see if an input mode was
    selected. The constants for them are no also in their own scope, so
    you write things like PinConfig::Mode::DISCONNECTED, which is more
    clear than before. It seems like this would be a good usecase to use
    C++11 enum classes to add scoping and type safety, but you cannot add
    methods to enum classes. So this uses a struct with methods and an
    old-style enum, which fixes the scoping but is a bit less typesafe.

    This struct-wrapping does not add any storage overhead and minimal
    to no code size overhead.

This does not properly update the reports yet. They are somewhat
updated, but analog pins still only generate analog reports now and the
pin modes are replaced by the new pin configs directly, which have
different values. There should probably be a single mode report and a
single value report, instead of splitting through analog and digital,
but that's for a later commit.

This uses a single array of structs. By using bitfields, we shrink the
value to 10 bits (needed for analog measurements) and shrink the mode to
4 bits, allowing them both to fit into 2 bytes instead of 3.

This does mean that negative values are no longer possible, but these
were only set for the negative modes, in which case the value wasn't
used in the first place.
Before, it accidentally set PWM mode on the pin instead of setting it as
disconnected. pin.othersdisconnected did work as expected.
This makes a number of changes:
 - Split INPUT mode into INPUT_DIGITAL and INPUT_ANALOG. This allows
   using analog pins as digital pins as well. It requires explicitely
   choosing either of these modes, except when using pin.makeinput,
   which autoselects based on the pin type.

 - Rename and renumber the pin mode constants. This makes things a bit
   more consistent and by removing the negative values we allow using
   the upper bits of the mode as flags or other values.

 - Generalize the "pin mode" value into a "pin config" value, which
   contains the pin mode, combined with other values. Right now, this
   is only a flag for wether the pullup is enabled (which was previously
   part of the mode), but in the future flags like "reports enabled" or
   values like the analog reference to use can be included.

   This removes the pin.setmode command and replaces it with pin.config,
   which works similar. It accepts the following modes: disabled,
   disconnected, input_digital, input_analog, output_digital,
   output_pwm. Additionally, the value flag_pullup can be added to
   the input modes to enable pullups (defaulting to disabled). e.g.:

       pin.config("d8", output_digital)
       pin.config("a0", input_digital + flag_pullup)

 - When setting a pin to output_digital, it now gets written to LOW by
   default. Previously, the value would be unchanged, which meant that
   if the pullup was previously enabled, it would be HIGH, otherwise it
   would be LOW. When using pin.config, the value can be overridden
   (which is useful if you must go from input-with-pullup to output-high
   without going through output-low for some reason).

 - pin.makeinput no longer takes input or input_pullup as the second
   argument, but instead expects a 0 or 1 value to disable/enable
   pullups. Leaving it out makes the default depend on the pin type (see
   next item).

 - When setting an analog pin to input using pin.makeinput, it no longer
   defaults to having the pullup enabled. Since the pullup is connected
   to 3.3V, that's too high for the ADC, and if the voltage sensed comes
   from a voltage divider, having the pullup enabled will even influence
   the values read.

 - PinConfig and Mode values are now wrapped in structs, so you can do
   things like config.mode().input() to see if an input mode was
   selected. The constants for them are no also in their own scope, so
   you write things like PinConfig::Mode::DISCONNECTED, which is more
   clear than before. It seems like this would be a good usecase to use
   C++11 enum classes to add scoping and type safety, but you cannot add
   methods to enum classes. So this uses a struct with methods and an
   old-style enum, which fixes the scoping but is a bit less typesafe.

   This struct-wrapping does not add any storage overhead and minimal
   to no code size overhead.

This does not properly update the reports yet. They are somewhat
updated, but analog pins still only generate analog reports now and the
pin modes are replaced by the new pin configs directly, which have
different values. There should probably be a single mode report and a
single value report, instead of splitting through analog and digital,
but that's for a later commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant