Additional sleep methods with array of pins as parameters ?

[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 :grin:

[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.