Additional sleep methods with array of pins as parameters ?

[Nca78]

Hello,

is there any plan to add new sleep methods that would take an array of pins instead of the 2 interrupt pins ?
Use cases would be :

• have more than 2 interrupts on atmega and more flexibility on pin numbers, this would use pin change interrupts
• be able to make battery powered sensors on NRF51 as hardware is still bugged even in the last versions of the chip. So it's either LPCOMP (but that limits to one interrupt pin only and only on pins 1-8) or PORT mode with pin sense which has a similar behavior than pin change on atmega (one interrupt for all activated pins).

So it looks like a good opportunity to kill two birds with one stone: have an "official" implementation of pin change interrupt for atmega, and unblock nrf51 users and probably also nrf52 users who need many interrupts.
Of course this need some "preparation" of the data so that it's easier for user to know what pin generated the interrupt. I'm not sure what is the best solution for this, app_gpiote.c from nrf5 SDK returns 2 masks for low=>high and high=>low pins but it doesn't seem to be so newbie-friendly.

If core team and/or @d00616 (for nrf5 part) think it's a good idea and we agree on how it's best to implement it I'm ok to prepare some pull requests.

[mfalkvidd]

@nca78 I think functionality like that would be nice. As you've already identified, there needs to be some user-friendly way to know which interrupt was triggered. I guess there also needs to be a way to specify the type of interrupt for each pin.

Arduino's attachInterrupt is a pretty clean way. You call it as many times as you like, with the pins you want. So no need to handle arrays. The downside is that the user needs to write their own interrupt handler. There are lots of no-no's in interrupt handlers (don't print to serial, millis() wont increment, etc) so it would be nice if we can avoid forcing people to learn all that.

Code could look like this maybe

void setup() {
  myAttachInterrupt(digitalPinToInterrupt(pin1), LOW));
  myAttachInterrupt(digitalPinToInterrupt(pin2), CHANGE));
  myAttachInterrupt(digitalPinToInterrupt(pin3), HIGH));
}

void loop() {
  Serial.println("Waiting for interrupt or sleep time expiring");
  int8_t result = sleep(SLEEP_TIME);
  switch (result) {
    case MY_WAKE_UP_BY_TIMER:
      // Woke up by timer
      // Do periodic stuff here
      break;
    case MY_SLEEP_NOT_POSSIBLE:
      // Unable to sleep
      break;
    case digitalPinToInterrupt(pin1):
      // pin1 was triggered
      break;
    case digitalPinToInterrupt(pin2):
      // pin2 was triggered
      break;
    case digitalPinToInterrupt(pin3):
      // pin3 was triggered
      break;
  }
}

sleep already returns the interrupt number that triggered the wakeup, so as long as all boards use interrupts between 0 and 127 there should be no need to change the return behavior.

[Yveaux]

@mfalkvidd beware, there is some tricky stuff with interrupt dis- & enabling around the AVR arduino interrupt attachment in the current code, that fixes a nasty bug that has been there for ages : https://github.com/mysensors/MySensors/blob/development/hal/architecture/AVR/MyHwAVR.cpp#L161

[mfalkvidd]

@yveaux wouldn't it be sufficient to loop over all enabled interrupts? Just like the current code does, except that the current code is hardcoded to 2 interrupts.

[Yveaux]

@mfalkvidd it probably will, but then the interrupthandlers need to stay attached during wake time.
The tricky stuff is in the attaching and detaching of the handlers

[mfalkvidd]

@yveaux I see. We would need to have functions wakeUp1(), wakeUp2(), ..., wakeUpN(). That's a mess.

[Yveaux]

@mfalkvidd that too, but maybe we can think of a smart solution to it, eg determine on runtime what the cause of the interrupt was, instead of having a separate handler for each interrupt cause.

[Nca78]

It's not a good idea to mix different types of interrupts I think and it will make everything messy.
My idea was to keep existing methods as they are and add another one using pin change interrupt on atmega or pin sense on nrf5 (and similar stuff on other ics if they support it).

app_gpiote on nrf5 receives 2 lists of pins to watch, one for high>low and one for low>high transition.
Based on current status of input when function is called it will activate sense for low or for high level.
It's using a mask to keep initial status of pins, and when interrupt is called it compares current status of the pins to what it saved and returns 2 masks with pins that made a low>high change and pins that made a high>low change.
With a function checking pin number inside those masks it's easy for user to check what pin has changed or not.

A way to do it, and matching idea of a "myAttachInterrupt" method from @mfalkvidd, would be to have something like that:

/**
* Result of the sleep method
*  used in one static variable
*/
typedef struct {
  // result of sleep function: error code or constant for timer/pin interrupt
  uint8_t sleepResult;
  // elapsed time during sleep
  //  for atmega we can give a rough estimate here based on number of loops done in hwInternalSleep
  //   so in case of interrupt + interval sending of data we can keep interval at +/- 8s "precision"
  uint32_t elapsedTime;
  // mask with pins that changed from low to high
  uint32_t changedPinsLowToHigh; 
  // mask with pins that change from high to low
  uint32_t changedPinsHighToLow; 
} mySleepResult;

/**
* Configuration for the sleep method 
* used in one static variable
*/
typedef struct {
  uint32_t watchPinsLowToHigh; // mask with pins with either HIGH or CHANGE mode
  uint32_t watchPinsHighToLow; // mask with pins with either LOW or CHANGE mode
  uint32_t pinStatusBeforeSleep; // mask with pin status before going to sleep
} mySleepConfig;


/**
 * Sleep function, returns error code or source of interrupt
 */
uint8_t advancedSleep(uint32_t ms);


/**
 * Adds one pin to watch to the masks, based on the mode
 *  returns success or error (invalid pin number, invalid mode, ...)
 */
uint8_t addPinToWatch(uint8_t pin, uint8_t mode);

/**
 * Checks changed status of one specific pin
 *  returns error code or something like PIN_CHANGE_LOWTOHIGH, PIN_CHANGE_HIGHTOLOW, PIN_NOT_CHANGED
 */
uint8_t checkPinChange(uint8_t pin);

/**
 * Removes pin to watch from the mySleepConfig masks
 */
void removePinToWatch(uint8_t pin);

/**
 * Resets mySleepConfig structure
 */
void clearSleepConfig(void);

/**
 * Returns sleep elapsed time in ms
 */
uint32_t getSleepTime(void);

[Nca78]

Any remarks on this anyone ? Should I make some kind of "demo" of it with my nrf51 code to make my idea more clear ?
If my suggestion above is stupid, don't hesitate to say it too, I'll accept it

[mfalkvidd]

@nca78 I'm more interested in how it would be used by sketch developers than the internal library design.

How would I use the new feature from my sketch?

[Nca78]

@mfalkvidd said in Additional sleep methods with array of pins as parameters ?:

@nca78 I'm more interested in how it would be used by sketch developers than the internal library design.

How would I use the new feature from my sketch?

A bit like how it's done in the script I posted here:
https://forum.mysensors.org/topic/6961/nrf5-bluetooth-action/1514

You can trigger on any pin you want and know which pin triggered interrupt:

  if (interrupt_cause == PIN_BUTTON1) {
    // if door status changed, we send door message
    if (door_status != last_sent_value) {
      sendDoorStatus();
    }
  }
  else if (interrupt_cause == PIN_BUTTON2) {
    if (millis() < last_button2_event || (millis() - last_button2_event > 100)) {
      last_button2_event = millis();
      blinkityBlink(2, 3); // not so useful, just for testing :)
    }
  }
  else {  // end of sleeping period, we send battery level
    sendBatteryStatus();
  }

I can add any pin, at the moment I only have a hall sensor and a button, but I will also wake up on interrupts from light sensor, 2 different interrupts from accelerometer etc.
Pin change allows the same thing on atmega328.

[mfalkvidd]

So there would be a global variable called interrupt_cause? If that's the case, it should probably be renamed to latest_interrupt_cause since it may be updated at any time.

Would there be a return value of sleep() that tells whether the user should check this global variable, or how would they know to check it?

[Nca78]

@mfalkvidd said in Additional sleep methods with array of pins as parameters ?:

So there would be a global variable called interrupt_cause? If that's the case, it should probably be renamed to latest_interrupt_cause since it may be updated at any time.

Would there be a return value of sleep() that tells whether the user should check this global variable, or how would they know to check it?

No that's just in my code for nrf51 that is just some kind of "proof of concept" that pin sense works with MySensors and that it solves the hardware bug creating a 1ma power consumption with other methods. Of course all of that must be wrapped into much more user friendly methods if included in MySensors.
I think a "mySleepResult" structure like in my previous message (https://forum.mysensors.org/topic/9234/additional-sleep-methods-with-array-of-pins-as-parameters/8) is a better idea, with all information about the sleep summed up in one place, with some additional helper functions to check if a specific pin has changed (and in which direction) or not. If we don't do that it will be too complicated for the "non-programmer" user to retrieve information.

For user the basic process to use it would be :

• I declare the pins I want to watch (in theory as many pins as wanted)
• I call the "advancedSleep" (or whatever name we give it) method
• I get a simple return value (similar to now) telling me if there was an error, if it's the end of sleep duration or give me the (first) pin that triggered the wake up
• If I want to get more detailed information (direction of pin change, check multiple pins, duration of sleep to restart sleeping only for remaining time in original delay, ...) then I can check the global instance of the structure (MySleepResult) or call the helper methods.

[NeverDie]

@nca78 said in Additional sleep methods with array of pins as parameters ?:

@mfalkvidd said in Additional sleep methods with array of pins as parameters ?:

So there would be a global variable called interrupt_cause? If that's the case, it should probably be renamed to latest_interrupt_cause since it may be updated at any time.

Would there be a return value of sleep() that tells whether the user should check this global variable, or how would they know to check it?

No that's just in my code for nrf51 that is just some kind of "proof of concept" that pin sense works with MySensors and that it solves the hardware bug creating a 1ma power consumption with other methods. Of course all of that must be wrapped into much more user friendly methods if included in MySensors.
I think a "mySleepResult" structure like in my previous message (https://forum.mysensors.org/topic/9234/additional-sleep-methods-with-array-of-pins-as-parameters/8) is a better idea, with all information about the sleep summed up in one place, with some additional helper functions to check if a specific pin has changed (and in which direction) or not. If we don't do that it will be too complicated for the "non-programmer" user to retrieve information.

For user the basic process to use it would be :

• I declare the pins I want to watch (in theory as many pins as wanted)
• I call the "advancedSleep" (or whatever name we give it) method
• I get a simple return value (similar to now) telling me if there was an error, if it's the end of sleep duration or give me the (first) pin that triggered the wake up
• If I want to get more detailed information (direction of pin change, check multiple pins, duration of sleep to restart sleeping only for remaining time in original delay, ...) then I can check the global instance of the structure (MySleepResult) or call the helper methods.

I'm glad you're working on this!

For possible inspiration, there is precedent for calling a function/method for each pin that you want to set as an interrupt. For instance, for regular arduino's, the call might be:

  pciSetup(7);
  pciSetup(8);
  pciSetup(9);
  pciSetup(A0);

as illustrated by the sketch at: https://playground.arduino.cc/Main/PinChangeInterrupt

That said, I'm entirely neutral about how it gets done.

[Nca78]

@neverdie thank you for the link I didn't remember about that, I will have a look.

[alexsh1]

@mfalkvidd said

myAttachInterrupt(digitalPinToInterrupt(pin2), CHANGE));

Sorry to chip in, but what's myAttachInterrupt function please?
Do you also have to do:

pinMode(pin2, INPUT);
myAttachInterrupt(digitalPinToInterrupt(pin2), CHANGE));

[mfalkvidd]

@alexsh1 chipping in is great I don’t know what would be in it, I was just outlining how using the new feature could look like from a sketch developer’s perspective.

I don’t think sketch developer should need to worry about setting the pin to input, except maybe if they want to activate a pullup/pulldown feature (if the mcu supports it).

[alexsh1]

@mfalkvidd OK, I was concerned it was something already implemented and I did not know about it.
I'd rather leave it as is. I know the idea is to avoid interrupt handlers, but we need to re-think and re-write the sketch code so that we only use volatile variables for true/false statements.

[Yveaux]

@mfalkvidd said in Additional sleep methods with array of pins as parameters ?:

I don’t think sketch developer should need to worry about setting the pin to input,

Yes, they have to set it to input. There's no pin configuration in the (AVR) sleep code: https://github.com/mysensors/MySensors/blob/development/hal/architecture/AVR/MyHwAVR.cpp#L153

[Nca78]

@yveaux said in Additional sleep methods with array of pins as parameters ?:

Yes, they have to set it to input. There's no pin configuration in the (AVR) sleep code: https://github.com/mysensors/MySensors/blob/development/hal/architecture/AVR/MyHwAVR.cpp#L153

But atmega pins default as inputs so it's not really mandatory with the default IC.

[Yveaux]

@nca78 said in Additional sleep methods with array of pins as parameters ?:

@yveaux said in Additional sleep methods with array of pins as parameters ?:

Yes, they have to set it to input. There's no pin configuration in the (AVR) sleep code: https://github.com/mysensors/MySensors/blob/development/hal/architecture/AVR/MyHwAVR.cpp#L153

But atmega pins default as inputs so it's not really mandatory with the default IC.

I like to make things explicit, especially if you decide to port to another architecture later on

[Nca78]

@yveaux said in Additional sleep methods with array of pins as parameters ?:

I like to make things explicit, especially if you decide to port to another architecture layer on

I completely agree with you. Just stating that @mfalkvidd is not wrong when saying at the moment it's not necessary to declare pins as inputs

To go back to the main subject should I make a first version of this code (in post 9) to have a clearer basis for future discussions ? Or am I going a wrong way ?

[NeverDie]

Anything is always better than nothing.

[mfalkvidd]

@yveaux clarification: imho, if support for many interrupts is added in a future version of MySensors (which is what we're discussing in this thread), sketch developers should not need to manually call pinMode.