Skip to content

Use +512 for 0x43 and +256 for 0x44 expander pins #847

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
tyeth wants to merge 3 commits into
base: add-arduino-nesso-n1
Choose a base branch
from
+68 −109
Open

Use +512 for 0x43 and +256 for 0x44 expander pins #847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ba954a
Use +512 for 0x43 and +256 for 0x44 expander pins
tyeth Dec 4, 2025
73b6392
Rely on BSP for battery WDT
tyeth Dec 4, 2025
2044971
fix gpio event, drop unnecessary overloads, formatting
tyeth Dec 4, 2025
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
44 changes: 4 additions & 40 deletions src/Wippersnapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,45 +100,9 @@ void Wippersnapper::provision() {

// Board specific initializations
Copy link
Member

@brentru brentru Dec 4, 2025

Choose a reason for hiding this comment

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

I don't feel provision() is an appropriate place for this - how about within _boards.h ?

Copy link
Member Author

@tyeth tyeth Dec 5, 2025

Choose a reason for hiding this comment

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

Yeah, that would probably be best.

Copy link
Member

@brentru brentru Dec 5, 2025

Choose a reason for hiding this comment

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

Great, let's refactor it

Copy link
Member Author

@tyeth tyeth Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#ifdef ARDUINO_ARDUINO_NESSO_N1
Wire.begin(SDA, SCL, 100000);

// verify chip id 0x49 at 0x0Ah
Wire.beginTransmission(0x49);
Wire.write(0x0A);
Wire.endTransmission();
Wire.requestFrom(0x49, 1);
uint8_t chipId = Wire.read();
if (chipId != 0x49) {
WS_DEBUG_PRINTLN("ERROR: AW32001E not found on I2C bus!");
Wire.endTransmission();
Wire.end();
} else {
WS_DEBUG_PRINTLN("AW32001E detected on I2C bus.");
// Disable AW32001E watchdog timer, read 05h, & 0x1F, write back
Wire.beginTransmission(0x49);
Wire.write(0x05);
Wire.endTransmission();
Wire.requestFrom(0x49, 1);
uint8_t regVal = Wire.read();
Wire.endTransmission();
WS_DEBUG_PRINTLN("AW32001E WDT reg before disable: " + String(regVal, BIN));
delay(10);
regVal &=
0b00011111; // Clear bits 5:6 to disable Watchdog timer, 7 for discharge
Wire.beginTransmission(0x49);
Wire.write(0x05);
Wire.write(regVal);
Wire.endTransmission();
Wire.end();
delay(10);

battery.enableCharge();
}

// // digitalWrite(LORA_ENABLE, FALSE);
// // digitalWrite(LORA_LNA_ENABLE, FALSE);
// // digitalWrite(GROVE_POWER_EN, TRUE);
// delay(10);
battery.begin();
delay(10);
battery.enableCharge();
#endif

// Initialize the status LED for signaling FS errors
Expand Down Expand Up @@ -1703,7 +1667,7 @@ void cbDisplayMessage(char *data, uint16_t len) {
/****************************************************************************/
bool Wippersnapper::encodePinEvent(
wippersnapper_signal_v1_CreateSignalRequest *outgoingSignalMsg,
uint8_t pinName, int pinVal) {
uint16_t pinName, int pinVal) {
bool is_success = true;
outgoingSignalMsg->which_payload =
wippersnapper_signal_v1_CreateSignalRequest_pin_event_tag;
Expand Down
24 changes: 23 additions & 1 deletion src/Wippersnapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,28 @@ class DisplayController;

#ifdef ARDUINO_ARDUINO_NESSO_N1
static NessoBattery battery; ///< Nesso-N1 Battery instance

/**************************************************************************/
/*!
@brief Printable subclass of ExpanderPin for use with Serial.println()
Allows ExpanderPin to be used interchangeably with uint8_t pins
when printing to Serial.
*/
/**************************************************************************/
class WsExpanderPin : public ExpanderPin, public Printable {
Copy link
Member

@brentru brentru Dec 4, 2025

Choose a reason for hiding this comment

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

Is this only used for DigitalGPIO? Wouldn't it make more sense to sit within the DigitalGPIO class header than the application header?

Copy link
Member Author

@tyeth tyeth Dec 5, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@brentru brentru Dec 5, 2025

Choose a reason for hiding this comment

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

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

public:
WsExpanderPin(uint16_t _pin) : ExpanderPin(_pin) {}
WsExpanderPin(ExpanderPin ep)
: ExpanderPin(ep.pin | (ep.address == 0x44 ? 0x100 : 0x200)) {}

size_t printTo(Print &p) const override {
size_t n = p.print("0x");
n += p.print(address, HEX);
n += p.print("_");
n += p.print(pin);
return n;
}
};
#endif

/**************************************************************************/
Expand Down Expand Up @@ -322,7 +344,7 @@ class Wippersnapper {
// Encodes a pin event message
bool
encodePinEvent(wippersnapper_signal_v1_CreateSignalRequest *outgoingSignalMsg,
uint8_t pinName, int pinVal);
uint16_t pinName, int pinVal);

// Pin configure message
bool configureDigitalPinReq(wippersnapper_pin_v1_ConfigurePinRequest *pinMsg);
Expand Down
2 changes: 1 addition & 1 deletion src/Wippersnapper_Boards.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@
#define BOARD_ID "arduino-nesso-n1"
#define USE_LITTLEFS
#define USE_STATUS_LED
#define STATUS_LED_PIN LED_BUILTIN
#define STATUS_LED_PIN LED_BUILTIN // can be +512/+256 as expander (0x43/44)
#define STATUS_LED_INVERTED
#elif defined(ARDUINO_ADAFRUIT_FEATHER_ESP32C6)
#define BOARD_ID "feather-esp32c6"
Expand Down
78 changes: 33 additions & 45 deletions src/components/digitalIO/Wippersnapper_DigitalGPIO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,13 @@ Wippersnapper_DigitalGPIO::~Wippersnapper_DigitalGPIO() {
/*******************************************************************************************************************************/
void Wippersnapper_DigitalGPIO::initDigitalPin(
wippersnapper_pin_v1_ConfigurePinRequest_Direction direction,
uint8_t pinName, float period,
uint16_t pinName, float period,
wippersnapper_pin_v1_ConfigurePinRequest_Pull pull) {
#if defined(ARDUINO_ARDUINO_NESSO_N1)
WsExpanderPin flexPinName = (ExpanderPin)pinName;
#else
uint8_t flexPinName = (uint8_t)pinName;
#endif
if (direction ==
wippersnapper_pin_v1_ConfigurePinRequest_Direction_DIRECTION_OUTPUT) {

Expand All @@ -70,14 +75,14 @@ void Wippersnapper_DigitalGPIO::initDigitalPin(
// if (String("D") + pinName == STATUS_LED_PIN.pin)
// #else
// deinit status led, use it as a dio component instead
if (pinName == STATUS_LED_PIN)
if (flexPinName == STATUS_LED_PIN)
releaseStatusLED();
#endif
#endif
pinMode(pinName, OUTPUT);
pinMode(flexPinName, OUTPUT);

WS_DEBUG_PRINT("Configured digital output pin on D");
WS_DEBUG_PRINTLN(pinName);
WS_DEBUG_PRINTLN(flexPinName);

// Initialize LOW
#if defined(ARDUINO_ESP8266_ADAFRUIT_HUZZAH) // not until we support
Expand All @@ -91,20 +96,20 @@ void Wippersnapper_DigitalGPIO::initDigitalPin(
digitalWrite(pinName, LOW);
}
#else
pinMode(pinName, OUTPUT);
digitalWrite(pinName, LOW); // initialize LOW
pinMode(flexPinName, OUTPUT);
digitalWrite(flexPinName, LOW); // initialize LOW
#endif
} else if (
direction ==
wippersnapper_pin_v1_ConfigurePinRequest_Direction_DIRECTION_INPUT) {
WS_DEBUG_PRINT("Configuring digital input pin on D");
WS_DEBUG_PRINT(pinName);
WS_DEBUG_PRINT(flexPinName);

if (pull == wippersnapper_pin_v1_ConfigurePinRequest_Pull_PULL_UP) {
WS_DEBUG_PRINTLN("with internal pull-up enabled");
pinMode(pinName, INPUT_PULLUP);
pinMode(flexPinName, INPUT_PULLUP);
} else {
pinMode(pinName, INPUT);
pinMode(flexPinName, INPUT);
WS_DEBUG_PRINT("\n");
}

Expand Down Expand Up @@ -142,7 +147,7 @@ void Wippersnapper_DigitalGPIO::initDigitalPin(
/********************************************************************************************************************************/
void Wippersnapper_DigitalGPIO::deinitDigitalPin(
wippersnapper_pin_v1_ConfigurePinRequest_Direction direction,
uint8_t pinName) {
uint16_t pinName) {
WS_DEBUG_PRINT("Deinitializing digital pin ");
WS_DEBUG_PRINTLN(pinName);

Expand Down Expand Up @@ -182,24 +187,17 @@ void Wippersnapper_DigitalGPIO::deinitDigitalPin(
*/
/********************************************************************/
int Wippersnapper_DigitalGPIO::digitalReadSvc(int pinName) {
// Service using arduino `digitalRead`
int pinVal = digitalRead(pinName);
return pinVal;
}

#if defined(ARDUINO_ARDUINO_NESSO_N1)
/********************************************************************/
/*!
@brief High-level digitalRead service impl. which performs a
digitalRead.
@param pin
The ExpanderPin instance
@returns The pin's value.
*/
/********************************************************************/
int Wippersnapper_DigitalGPIO::digitalReadSvc(ExpanderPin pin) {
// probably this should be >255 instead for future use.
if ((pinName & 0x100) || (pinName & 0x200)) {
WsExpanderPin flexPinName = (ExpanderPin)pinName;
// Service using expander `digitalRead`
int pinVal = digitalRead(flexPinName);
return pinVal;
}
#endif
// Service using arduino `digitalRead`
int pinVal = digitalRead(pin);
int pinVal = digitalRead(pinName);
return pinVal;
}

Expand All @@ -212,25 +210,8 @@ int Wippersnapper_DigitalGPIO::digitalReadSvc(ExpanderPin pin) {
The pin's value.
*/
/*******************************************************************************/
void Wippersnapper_DigitalGPIO::digitalWriteSvc(ExpanderPin pin, int pinValue) {
WS_DEBUG_PRINT("Digital Pin Event: Set ");
WS_DEBUG_PRINT(pin.pin);
WS_DEBUG_PRINT(" to ");
WS_DEBUG_PRINTLN(pinValue);
digitalWrite(pin, pinValue);
}
#endif

/*******************************************************************************/
/*!
@brief Writes a value to a pin.
@param pinName
The pin's name.
@param pinValue
The pin's value.
*/
/*******************************************************************************/
void Wippersnapper_DigitalGPIO::digitalWriteSvc(uint8_t pinName, int pinValue) {
void Wippersnapper_DigitalGPIO::digitalWriteSvc(uint16_t pinName,
int pinValue) {
WS_DEBUG_PRINT("Digital Pin Event: Set ");
WS_DEBUG_PRINT(pinName);
WS_DEBUG_PRINT(" to ");
Expand All @@ -244,6 +225,13 @@ void Wippersnapper_DigitalGPIO::digitalWriteSvc(uint8_t pinName, int pinValue) {
digitalWrite(pinName, !pinValue);
else
digitalWrite(pinName, pinValue);
#elif defined(ARDUINO_ARDUINO_NESSO_N1)
if ((pinName & 0x100) || (pinName & 0x200)) {
WsExpanderPin flexPinName = (ExpanderPin)pinName;
digitalWrite(flexPinName, pinValue);
} else {
digitalWrite(pinName, pinValue);
}
#else
digitalWrite(pinName, pinValue);
#endif
Expand Down
29 changes: 7 additions & 22 deletions src/components/digitalIO/Wippersnapper_DigitalGPIO.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@

/** Holds data about a digital input pin */
struct digitalInputPin {
uint8_t pinName; ///< Pin name
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
Copy link
Member

@brentru brentru Dec 4, 2025

Choose a reason for hiding this comment

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

This change is throughout the API, is it necessary? What's the reason for the change?

Copy link
Member Author

@tyeth tyeth Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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){};

Copy link
Member

@brentru brentru Dec 5, 2025

Choose a reason for hiding this comment

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

Do we need this right now? Could we switch to uint16_t in API v2 codebase, instead?

Copy link
Member Author

@tyeth tyeth Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

@brentru brentru Dec 8, 2025

Choose a reason for hiding this comment

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

The idea is to move away from uint8_t pin numbers and towards uint16_t pin numbers.

If you want to do this, it can potentially break the API and will need more testing. I'd like to see a separate PR opened for moving to uint16_t, rather than keeping that critical (and possibly breaking) change in this PR.

long period; ///< Timer interval, in millis, -1 if disabled.
long prvPeriod; ///< When timer was previously serviced, in millis
int prvPinVal; ///< Previous pin value
};

// forward decl.
Expand All @@ -42,28 +42,13 @@ class Wippersnapper_DigitalGPIO {

void
initDigitalPin(wippersnapper_pin_v1_ConfigurePinRequest_Direction direction,
uint8_t pinName, float period,
uint16_t pinName, float period,
wippersnapper_pin_v1_ConfigurePinRequest_Pull pull);
void
deinitDigitalPin(wippersnapper_pin_v1_ConfigurePinRequest_Direction direction,
uint8_t pinName);

uint16_t pinName);
int digitalReadSvc(int pinName);
void digitalWriteSvc(uint8_t pinName, int pinValue);
#if defined(ARDUINO_ARDUINO_NESSO_N1)
// void
// initDigitalPin(wippersnapper_pin_v1_ConfigurePinRequest_Direction
// direction,
// ExpanderPin pinName, float period,
// wippersnapper_pin_v1_ConfigurePinRequest_Pull pull);
// void
// deinitDigitalPin(wippersnapper_pin_v1_ConfigurePinRequest_Direction
// direction,
// ExpanderPin pinName);

int digitalReadSvc(ExpanderPin pinName);
void digitalWriteSvc(ExpanderPin pinName, int pinValue);
#endif
void digitalWriteSvc(uint16_t pinName, int pinValue);
void processDigitalInputs();

digitalInputPin *_digital_input_pins; /*!< Array of gpio pin objects */
Expand Down
Loading