Use +512 for 0x43 and +256 for 0x44 expander pins #847
Conversation
|
So, this adds the buttons, they work, it's based on the expander pins constructor which does a check of the uint16 pin number & 0x100 (+256 for the 2nd expander at 0x44), and then I've used & 0x200 (512) to represent the other expander for now (at 0x43). Anyway, I've added the pins in board config as D512 and D513, on staging, and the buttons work with pull ups. It uses the latest master branch of arduino-esp32, branched in our fork as wipper-3.3.5-pre-SHA. The status pixel is defined as an ExpanderPin for now rather than the Dxxx syntax, but it could be a user pixel in theory. |
|
@brentru as I had already done most of the work in an uncommitted branch, I've finished it off here and saved it for review / v2 inspiration. |
| long period; ///< Timer interval, in millis, -1 if disabled. | ||
| long prvPeriod; ///< When timer was previously serviced, in millis | ||
| int prvPinVal; ///< Previous pin value | ||
| uint16_t pinName; ///< Pin name |
There was a problem hiding this comment.
This change is throughout the API, is it necessary? What's the reason for the change?
There was a problem hiding this comment.
The idea is to move away from uint8_t pin numbers and towards uint16_t pin numbers.
Effectively combining the idea of few board BSPs (pico) already using uin16_t, and the ExpanderPins constructor which takes a uint16_t, to allow use of an unreserved set of pins for any gpio's built-in but not directly on the main mcu. I'd also noticed we use int instead for pin number in many places.
0-255 (uint8_t) are effectively then reserved for the main boards pins (including any bsp related higher ranges like the LED on PicoW controlled via CYW43 wifi coprocessor at "virtual" pin 64).
Ranges above 255 are expander pins, be those defined by the BSP (Nesso) or ourselves (Memento).
Mainly it was an easy way to switch code path for expander pins, and between each expander, with similar logic to the ExpanderPins constructor:
ExpanderPin(uint16_t _pin) : pin(_pin & 0xFF), address(_pin & 0x100 ? 0x44 : 0x43){};
There was a problem hiding this comment.
Do we need this right now? Could we switch to uint16_t in API v2 codebase, instead?
There was a problem hiding this comment.
We would instead then have to manually define every pin number translation in the firmware, to map a custom pin number within the uint8_t range (call it 240 and 241 for the two buttons) to the ExpanderPin i2c addresses of 0x44 and channels of 0 and 1 respectively.
The board def would have two pins at 240 and 241 to keep within uint8_t.
Instead of currently 256 and 257, or 512/513 depending on extender at 0x44 or 0x43, which conveniently align (and therefore automatically can map) to the ExpanderPin constructor where the i2c address is based on & 0x100 (or &256), and then the channel is from pin number & 0xFF (so just the last two bytes of uint8_t being 0 or 1 in the buttons case)
| // Obtain device's MAC address | ||
| getMacAddr(); | ||
|
|
||
| // Board specific initializations |
There was a problem hiding this comment.
I don't feel provision() is an appropriate place for this - how about within _boards.h ?
There was a problem hiding this comment.
Yeah, that would probably be best.
There was a problem hiding this comment.
Ah, no I totally forgot, the reason I did it there was so the flash has initialised before the battery charger kicks in, instead of battery charger first and possibly causing a brownout for the flash begin / write afterwards.
Not sure that logic is sound to be fair, probably worth just shifting it out.
| when printing to Serial. | ||
| */ | ||
| /**************************************************************************/ | ||
| class WsExpanderPin : public ExpanderPin, public Printable { |
There was a problem hiding this comment.
Is this only used for DigitalGPIO? Wouldn't it make more sense to sit within the DigitalGPIO class header than the application header?
There was a problem hiding this comment.
Currently it's only to add compatibility for one debug print call of flexyPinName (ExpanderPin instance), which could instead print the original decimal pinName, and so this class could probably be removed entirely, but it potentially would be extended to the pixel/uart/pwm classes.
Having our own WsExpanderPin wrapper also means we can more easily extend the ExpanderPin struct/class (or our super class) to have an i2c bus number or any other board/arch specific functionality.
There was a problem hiding this comment.
y, but it potentially would be extended to the pixel/uart/pwm classes.
I agree with keeping it at the application (or a new component?) level for translation but if the debug print is the only feature right now, and you don't feel it's required, we could remove it entirely and add an issue for implementing a WsExpanderPin class within API v2.
The WsExpanderPin class and its instances could sit as a component and be able to translate between different classes
| long period; ///< Timer interval, in millis, -1 if disabled. | ||
| long prvPeriod; ///< When timer was previously serviced, in millis | ||
| int prvPinVal; ///< Previous pin value | ||
| uint16_t pinName; ///< Pin name |
There was a problem hiding this comment.
This change is throughout the API, is it necessary? What's the reason for the change?
stashing this here and seeing if latest bsp builds