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

VBUS - Low Power - Interrupt #125

Open
battosai30 opened this issue Apr 4, 2020 · 24 comments
Open

VBUS - Low Power - Interrupt #125

battosai30 opened this issue Apr 4, 2020 · 24 comments

Comments

@battosai30
Copy link

Hi,
I searched in all files and git issues but I could not find this information : is it possible to generate an interruption from VBUS ?
I think you understand why : I want to immediatly etablish a link with CMWX (in order to flash it or communicate with it) as he is in stop mode.
Thanks

@hallard
Copy link

hallard commented Apr 9, 2020

Well, just depending on which I/O pin is connected VBUS, so depending on your board.

check variants files of your board for STM32L0_CONFIG_PIN_VBUS defined symbol GPIO.

For GNAT board, you can see variant.h it's STM32L0_GPIO_PIN_PA8

then in variant.cpp you search for STM32L0_GPIO_PIN_PA8 and see it's D24 (comments are false last is 24 not 25) and it has IRQ capabilities PIN_ATTR_EXTI

So attachInterrupt(24, usb_handler, RISING); may do the trick. Never tested, so let us now

@GrumpyOldPizza
Copy link
Owner

I am still pondering that one.

Is it good enough to add 2 APIs:

USBDevice.enableHotPlug()
USBDevice.disableHotPlug()

Idea being that at reset the code detects whether USB is connected or not. After the initial determination it only checks if hotplug got enabled.

@hallard
Copy link

hallard commented May 2, 2020

Sot sure what's we are able to do on this point, you know better than I.

The idea is to be able to wake from sleep on hot plug and then do some things (like wait 5s to be able to flash the device or whatsoever )
Bu we need to know that we've been waked by hotplug, do able?

@GrumpyOldPizza
Copy link
Owner

Still toying with that. Key questions:

  • do you just need a way to say "please keep USB_VBUS detection in STOP mode"

  • do you need a way to detect via callback whether USB_VBUS has something attached to ?

  • do you need a way to detect via callback whether a USB host has connected to your USB device ?

N.b. it is possible that a battery charger attaches to your USB port. So depending upon whether you need to know that the answers to the last 2 questions might be interesting to me.

@battosai30
Copy link
Author

do you just need a way to say "please keep USB_VBUS detection in STOP mode"

It could be a solution and I think :

do you need a way to detect via callback whether USB_VBUS has something attached to ?

would be the same thing no ?

do you need a way to detect via callback whether a USB host has connected to your USB device ?

In my precise case, VBUS detection would do the trick, but It would be even better to detect a "real" USB connection ! But I guess it's not the same quantity of work ^^'

I precise that it's a custom board and I have PA8 well connected to VBUS from my USB connector.

@GrumpyOldPizza
Copy link
Owner

Mind checking out the code I dropped this evening on github ?

USBDevice:

bool begin();
void end();
bool attach();
bool detach();
void wakeup();

bool attached();
bool connected();
bool configured();
bool suspended();

void onConnect(void(*callback)(void));
void onDisconnect(void(*callback)(void));
void onSuspend(void(*callback)(void));
void onResume(void(*callback)(void));

void enableWakeup();
void disableWakeup();

begin() is called automatically as well as the first attach().

So in a nutshell, begin() sets up the whole internals and get USB_VBUS detection going. end() nukes everything. attach() attaches the protocol stack, so that when USB_VBUS gets detected, USB gets going. detach() removes the stack, but keeps USB_VBUS detection alive. wakeup() is the remote wakeup should you be in suspended USB bus mode.

attach() means USB_VBUS present, connected() means attached() and the USB stack active, configured() means that the USB host has set the USB configuration (speed, power ...), suspended() means the bus got suspended.

enableWakeup() means your USB_VBUS is tracked during STOP, and disableWakeup() disables that again.

@battosai30
Copy link
Author

Waow ! I will test that tomorrow ;)

Big thanks !

@hallard
Copy link

hallard commented May 7, 2020

Thomas,

Just to be sure to do the correct way

A)

  • On startup call enableWakeup() set onConnect() and onDisconnect() callbacks
    • if usb is connected it works as usual USBSerial
    • if not all USB stuff is disabled and set to low power

B)

  • if USB was connected at startup and we disconnect later
    • onDisconnect is fired, do we need to call end() or it's done by your end?

C)

  • if USB is reconnected when cpu is sleeping or not
    • onConnect is fired, we need to call attach() and wait for connected() and configured() to get all USB stuff correct?

@GrumpyOldPizza
Copy link
Owner

GrumpyOldPizza commented May 8, 2020

A) Yes. If USB_VBUS is present at startup everything works like before. The USB stack is running. Depending upon the timing you might call the USBDevice.onConnect before the USB_VBUS logic says that the signal is good. So that is race condition I perhaps need to avoid.

B) Yes, then onDisconnect() fires. The USB stack is stopped automatically. If you call USBDevice.end() it also nukes the USB_VBUS detection (which you may or may not want to do).

C) Yes, onConnect() fires, but the stack automaitcally gets going. No need to wait for anything.

Looks like I need to rename attach() / detach() to something else. The idea is that "attach()" is really "if USB_VBUS is present, start the stack, otherwise do so when USB_VBUS is detected in the future". "detach()" is really "if connected, stop the STACK, if not connected, disable the automatic start of the USB stack on detecting USB_VBUS".

The APIs are meant for 2 purposes:

With onConnect() and onDisconnect() you could control for example as to whether your sketch owns DOSFS, or USB/MSC.

With attach() / detach() you can control STOP mode. Remember, as long as USB is connected L0 cannot enter STOP mode (it lingers then in SLEEP). So if it matters, you could call USBDevice.detach() before you enter STOP (for a longer time) and USBDevice.attach() after you leave STOP mode.

@GrumpyOldPizza
Copy link
Owner

An alternative scheme would be that "setup()" gets called via "USBDevice.begin()" not not with "USBDevice.attach()". Only when SerialUSB.begin() is called USBDevice.attach() is called from there.

This would allow you then to install the callbacks before you call SerialUSB.begin().

However in that variant I needed to add a "USBMSC" class, so that a USBMSC.begin() would also call USBDevice.attach() as side-effect.

@hallard
Copy link

hallard commented May 8, 2020

Thomas,

With attach() / detach() you can control STOP mode. Remember, as long as USB is connected L0 cannot enter STOP mode (it lingers then in SLEEP). So if it matters, you could call USBDevice.detach() before you enter STOP (for a longer time) and USBDevice.attach() after you leave STOP mode.

Interesting, was not aware of. I saw when measuring power consumption that calling detach() was the trick to reduce consumption to minimal. I saw also that when I measure consumption when I upload and sketch start (so with usb connected) and few seconds later I remove USB the consumption was not going to "minimal'. Something related to USB stuff was still consuming.
So the only way to measure was then to reset the board with no usb connexion so the consumption was the one expected. Kris confirmed that measuring consumption should be done starting with no USB connected.
May be with these new features, it's solved, need to try that.

I'll try to write and post a sketch demonstrating this new USB Stuff, it will talk more than writing text.

@hallard
Copy link

hallard commented May 8, 2020

Thomas,

I tried with mitigated success this following sketch on a gnat board (but should work on any other as soon as VBUS is wired on the correct pin and defined in the variant).

It output debug one Serial and also on SerialUSB if it's connected of course

See my comments below

#include "STM32L0.h"

// Choose FTDI Serial debug Port
// On Gnat D9 (Serial3) connected to RX of FTDI to we can
// see debug even if no USB
#define SERIAL_DEBUG  Serial3

#define debug(VAL)    { SERIAL_DEBUG.print(VAL);    if (debug_USB) {Serial.print(VAL);} }
#define debugln(VAL)  { SERIAL_DEBUG.println(VAL);  if (debug_USB) {Serial.println(VAL);} }
#define debugflush()  { SERIAL_DEBUG.flush();       if (debug_USB) {Serial.flush();} }

void ledOn() { digitalWrite(LED_BUILTIN, LOW); }
void ledOff() { digitalWrite(LED_BUILTIN, HIGH); }

bool debug_USB ;
bool VUSB;
bool configuredUSB;
bool connectedUSB;
bool suspendedUSB;
volatile bool changedUSB;

void usbSuspend()   
{ 
  debugln("** USB Suspend **");     
}

void usbResume() 
{ 
  debugln("** USB Resume **"); 
}

void usbConnect()   
{ 
  debugln("** USB Connected **"); 
  changedUSB = true;     
}

void usbDisconnect()
{ 
  debugln("** USB Disconnected **"); 
  changedUSB = true;
}

bool manageUSBState() 
{ 
  bool _usb = USBDevice.attached();
  connectedUSB = USBDevice.connected();
  configuredUSB = USBDevice.configured();
  suspendedUSB = USBDevice.suspended();

  // Enable debug output thru USB port if all is fine
  if ( configuredUSB ) {
    debug_USB = true;
  } else {
    debug_USB = false;
  }

  debug("VUSB attached="); debug(_usb);
  debug(" connected=");  debug(connectedUSB);
  debug(" configured="); debug(configuredUSB);
  debug(" suspended=");  debug(suspendedUSB);
  debug(" serialUSB="); debugln(debug_USB);

  return _usb;
}

void setup() 
{
  debug_USB = false;

  pinMode( LED_BUILTIN, OUTPUT);
  ledOff();

  SERIAL_DEBUG.begin(115200);
  while(!SERIAL_DEBUG);
  debugln("\r\n*** Setup Serial Debug OK ***");

  USBDevice.onConnect(usbConnect);
  USBDevice.onDisconnect(usbDisconnect);
  USBDevice.onSuspend(usbSuspend);
  USBDevice.onResume(usbResume);

  // Don't try to get USBDevice.attached() it's false
  // all related stuff may not be ready so classic wait 
  // USB serial port ready (or not) need to be done
  // Wait connected in 8s max
  Serial.begin(115200);
  while (!Serial && millis()<=8000 ) {
      ledOn();
      delay(10);
      ledOff();
      delay(750);
  }

  if (USBDevice.configured() ) {
    debug_USB = true;
    debugln("*** Serial USB OK ***");
  } else {
    debugln("*** Serial USB not configured ***");
  }

  // Enable usb plug to wake up
  USBDevice.enableWakeup();
}

void loop() 
{
  // Get and show USB State
  if (changedUSB) { 
    changedUSB = false;
    debugln("USB Change");       
    VUSB = manageUSBState();

    if (VUSB) {
      ledOn();
    } else {
      ledOff();
    }
  } else {
    debugln ("timer");
  }

  debugln("Going to sleep...");
  debugflush();
  STM32L0.deepsleep(10000);
  debug("Waked by ");
} 

So with this sample everything is working fine I can plug and unplug the USB, it's detected.
BUT the callback usbConnect does not wakeup the CPU, thus when I plug the USB I'm waked after timer expiration only (in the trace below it's happing 10s after plug).

image

So except I need to wait to be able to wake and do stuff, all is working fine. And once waked debug goes thru Serial and SerialUSB fine because as you see in the logs USB is configured()

So I decided to change callback usbConnect() to add a wakeup() as follow

void usbConnect()   
{ 
  STM32L0.wakeup();
  debugln("** USB Connected **"); 
  changedUSB = true;     
}

And I'm waked immediately but then, I don't have anymore debug on USBSerial, looking at the logs looks like the resume is happening after and thus my internal flag debug_USB is not positioned that's why I don't have any data on SerialUSB, check log below

image

So I decided to fire the wakup() on the callback usbResume()

void usbResume() 
{ 
  STM32L0.wakeup();
  debugln("** USB Resume **"); 
  changedUSB = true;     
}

I'm waked after resume, but still no luck, usb never get configured(), looks like wakeup() may have a side effect?

image

@hallard
Copy link

hallard commented May 8, 2020

Ok, looks like I got it fixed added a delay in loop() after wakeup because of changedUSB and leaving the wakeup() into usbResume callback

  if (changedUSB) { 
    changedUSB = false;
    debugln("USB Change");       
    delay(2000);
    VUSB = manageUSBState();

    if (VUSB) {
      ledOn();
    } else {
      ledOff();
    }
  } else {
    debugln ("timer");
  }

I tried to replace this delay with a loop until configured() but sometimes it works sometime not, so the delay is the only thing working each time on my side;

here is the final sketch working on my side

#include "STM32L0.h"
#include "stm32l0_gpio.h"

// Choose FTDI Serial debug Port
// On Gnat D9 (Serial3) connected to RX of FTDI to we can
// see debug even if no USB
#define SERIAL_DEBUG  Serial3

#define debug(VAL)    { SERIAL_DEBUG.print(VAL);    if (debug_USB) {Serial.print(VAL);} }
#define debugln(VAL)  { SERIAL_DEBUG.println(VAL);  if (debug_USB) {Serial.println(VAL);} }
#define debugflush()  { SERIAL_DEBUG.flush();       if (debug_USB) {Serial.flush();} }

void ledOn() { digitalWrite(LED_BUILTIN, LOW); }
void ledOff() { digitalWrite(LED_BUILTIN, HIGH); }

bool debug_USB ;
bool VUSB;
bool configuredUSB;
bool connectedUSB;
bool suspendedUSB;
volatile bool changedUSB;

void usbSuspend()   
{ 
  debugln("** USB Suspend **");     
}

void usbResume() 
{ 
  STM32L0.wakeup();
  debugln("** USB Resume **"); 
  changedUSB = true;     
}

void usbConnect()   
{ 
  debugln("** USB Connected **"); 
}

void usbDisconnect()
{ 
  debugln("** USB Disconnected **"); 
  debug_USB = false;
  changedUSB = true;
}

bool manageUSBState() 
{ 
  bool _usb = USBDevice.attached();
  connectedUSB = USBDevice.connected();
  configuredUSB = USBDevice.configured();
  suspendedUSB = USBDevice.suspended();

  // Enable debug output thru USB port if all is fine
  debug_USB = configuredUSB;

  debug("USB attached="); debug(_usb);
  debug(" connected=");  debug(connectedUSB);
  debug(" configured="); debug(configuredUSB);
  debug(" suspended=");  debug(suspendedUSB);
  debug(" serialUSB="); debugln(debug_USB);

  return _usb;
}

void setup() 
{
  debug_USB = false;

  pinMode( LED_BUILTIN, OUTPUT);
  ledOff();

  SERIAL_DEBUG.begin(115200);
  while(!SERIAL_DEBUG);
  debugln("\r\n*** Setup Serial Debug OK ***");

  USBDevice.onConnect(usbConnect);
  USBDevice.onDisconnect(usbDisconnect);
  USBDevice.onSuspend(usbSuspend);
  USBDevice.onResume(usbResume);

  // Don't try to get USBDevice.attached() it's false
  // all related stuff may not be ready so classic wait 
  // USB serial port ready (or not) need to be done
  // Wait connected in 8s max
  Serial.begin(115200);
  while (!Serial && millis()<=8000 ) {
    ledOn(); delay(10); ledOff(); delay(750);
  }

  if (USBDevice.configured() ) {
    debug_USB = true;
    debugln("*** Serial USB OK ***");
  } else {
    debugln("*** Serial USB not configured ***");
  }

  // Enable usb plug to wake up
  USBDevice.enableWakeup();
}

void loop() 
{
  // Get and show USB State
  if (changedUSB) { 
    changedUSB = false;
    delay(2000);
  }

  VUSB = manageUSBState();
  if (VUSB) {
    ledOn();
  } else {
    ledOff();
  }

  debugln("Going to sleep...");
  debugflush();
  STM32L0.deepsleep(10000);
  debug("Waked by ");
  if (changedUSB) { 
    debug("Change");       
  } else {
    debug ("Timer");
  }
} 

@GrumpyOldPizza
Copy link
Owner

I need to recheck the code here. I ran into something similar yesterday where the code went throu a suspend/resume sequence, but did not properly restart up SerialUSB (and/or USB/MSC).

The USBDevice.configured() tracks directly the state of the USB Stack. So I am puzzeled why that would not show things properly.

With STM32L0.wakeup(). The rule is simple. If there is a callback and you have attached it, you need to call STM32L0.wakeup(). If there is a callback and you have nothing attached to it, it will wake you up. The idea is to allow calllback drive code that does not bail out of STM32L0.sleep() / STM32L0.deepsleep(). I am constantly forth and back on this one. On one hand that is a powerful tool. On the other hand, the set of events that will wake you up is so big to begin with that unless you attach callbacks to every single one, you'll be woken up most of the time anyway.

@hallard
Copy link

hallard commented May 8, 2020

Thomas,

I tested the consumption on gnat with the above sketch, the graph below is when It finished to start and setup of course. Then I removed USB. As you can see you have peak every 10s (it's the wakeup) and between then some small ones may be related to USB looking to happen every 200ms

image

now reseting the same board (with no usb connected), wait the setup to finish and here the new graph, as you ca see all "glitch" in between the 10s sleep are not present anymore

image

@hallard
Copy link

hallard commented May 8, 2020

Noticed something more,
once flashed with USB tru IDE (and leaving USB connected after) with GNSS gnat old code, before I was able to reflash with IDE when gnat was trying to fix (waked every second) but now, even if I'm not using any new USB code in this sketch I can't, I'm stuck to put the device in boot mode with boot and reset button even if my sketch is currently writing debug info on SerialUSB (that I can read on the serial console). Very strange

@nelisse
Copy link

nelisse commented May 8, 2020

Thomas: this description of the programming lock-up looks similar to a situation I noticed on my devices during the SerialProtocolTest.....

@GrumpyOldPizza
Copy link
Owner

Give me a day or 2. Need to rework some other parts of USB first.

@matthijskooijman
Copy link

I've recently also been fiddling with USB powersaving (not so much suspend/resume, but I needed to do some runtime detection of being attached or not and shutting down USB sufficiently during sleep). I was using 0.10.0 until now, but it seems these changes will really help my case, so I'll switch to git master next.

However, while reading the code and this issue, there's a few things that are still unclear, which I've listed below. Some of these might be misunderstanding on my side, but I'm asking anyway just in case there's a genuine bug in the code. Others are just my feedback on how I am (not) understanding the API as present, as input for improving that API.

  • It seems that bdd33df removed support for not having an VBUS pin? Was that intentional? I'm working with a board that has no way to detect VBUS (which, I think, might be a small USB spec violation and we'll probably fix this in the next revision, but I still have to work with the current board). It seems that this could be fairly easy to restore, though, just skip all pin handling and set usbd_attached to true if no pin is configured.
  • attached() returns whether VBUS is present, and is irregardless of whether USBDevice.attach() is called, which is a bit confusing. I see you were already considering renaming these functions in VBUS - Low Power - Interrupt #125 (comment), but maybe it would be better to rename attached() instead (to has_vbus() or something like that)? Since attach() / detach() do actually seem to do what you'd expect (if VBUS is present, if not, they set the attach/detach behavior for when VBUS is present later).
  • I'm not sure what connected() means exactly. The code says when usbd_state == USBD_STATE_CONNECTED, but if I read the code correctly, that state can be only set by HAL_PCD_ResumeCallback, so after a resume (WKUP IRQ). So unless the initial connection also results in a "resume" event, I think that connected() will not return true until after a suspend/resume cycle?
  • I'm thoroughly confused about what enableWakeup() and setVBUSDetect() do and how they interact. It seems that:
    • setVBUSDetect(SLEEP_AND_STOP) configures the VBUS GPIO pin work even when in STOP mode (i.e. allowing the device to "wake up" from stop mode). Using setVBUSDetect(SLEEP) is the default and disables this special configuration, allowing wakeup only from sleep mode (where all interrupts allow wakeup normally). VBUS detection seems to be always active run mode, which makes this API slightly confusing to me (but if you consider VBUS detection during run implied, then I guess this API makes sense).
    • enableWakeup() seems to ensure that any callbacks (suspend/resume/connect/disconnect) that are unset trigger STM32L0_SYSTEM_EVENT_APPLICATION, which interrupts an ongoing sleep operation. If you configure a callback, that callback must call STM32L0.wakeup() itself. Terminology is a bit confusing here, since "wakeup" here actually means "interrupt an ongoing sleep/stop cycle", rather than an actual low-level wakeup from sleep/stop (the latter will often trigger the former, but not neccesarily).
  • STM32L0.deepsleep() seems to do the same as sleep() (that is, it uses STM32L0_SYSTEM_POLICY_DEEPSLEEP, but AFAICS that is handled identical to STM32L0_SYSTEM_POLICY_SLEEP by stm32l0_system_sleep()?

An alternative scheme would be that "setup()" gets called via "USBDevice.begin()" not not with "USBDevice.attach()". Only when SerialUSB.begin() is called USBDevice.attach() is called from there.

That would seem elegant, because it would also prevent showing up as an USB device if you don't call SerialUSB.begin(). At the same time, that would prevent firmware uploads without BOOT0 fiddling when the sketch doesn't call SerialUSB.begin(), which probably isn't ideal.

@GrumpyOldPizza
Copy link
Owner

  • It seems that bdd33df removed support for not having an VBUS pin? Was that intentional? I'm working with a board that has no way to detect VBUS (which, I think, might be a small USB spec violation and we'll probably fix this in the next revision, but I still have to work with the current board). It seems that this could be fairly easy to restore, though, just skip all pin handling and set usbd_attached to true if no pin is configured.

That was intentional. A USB_VBUS pin is required going forward (or for STM32L4/L4+/WB detection via PVM1).

  • attached() returns whether VBUS is present, and is irregardless of whether USBDevice.attach() is called, which is a bit confusing. I see you were already considering renaming these functions in #125 (comment), but maybe it would be better to rename attached() instead (to has_vbus() or something like that)? Since attach() / detach() do actually seem to do what you'd expect (if VBUS is present, if not, they set the attach/detach behavior for when VBUS is present later).

Naming is always tricky. The USBDevice.attach() came in with the ArduinoCore-samd heritage. However this conflicts with the conventions that the USB spec uses, where "attach" means that the physical connection is established (connector plugged in, VBUS is present). If you have a better suggestion of how to name "USBDevice.attach()" I am all ears. This function really attaches the USB stack to the connector so to speak.

  • I'm not sure what connected() means exactly. The code says when usbd_state == USBD_STATE_CONNECTED, but if I read the code correctly, that state can be only set by HAL_PCD_ResumeCallback, so after a resume (WKUP IRQ). So unless the initial connection also results in a "resume" event, I think that connected() will not return true until after a suspend/resume cycle?

"connected" should be obvious. It means that the peer (host) has established a connection (bus reset issued, address configured). That is important, because it's possible to have a battery connected via USB. Guess I should add the proper BCD detection logic and add USBDevice.detected() which returns 0 while nothing is detected, and a non-zero when something is detected, and of course what ...

  • setVBUSDetect(SLEEP_AND_STOP) configures the VBUS GPIO pin work even when in STOP mode (i.e. allowing the device to "wake up" from stop mode). Using setVBUSDetect(SLEEP) is the default and disables this special configuration, allowing wakeup only from sleep mode (where all interrupts allow wakeup normally). VBUS detection seems to be always active run mode, which makes this API slightly confusing to me (but if you consider VBUS detection during run implied, then I guess this API makes sense).

USB_VBUS detection is per default disabled during STOP mode. That is why this API is there. The reason is that there is additional power draw on this pin if you enable detection. So there needed to be a choice.

  • enableWakeup() seems to ensure that any callbacks (suspend/resume/connect/disconnect) that are unset trigger STM32L0_SYSTEM_EVENT_APPLICATION, which interrupts an ongoing sleep operation. If you configure a callback, that callback must call STM32L0.wakeup() itself. Terminology is a bit confusing here, since "wakeup" here actually means "interrupt an ongoing sleep/stop cycle", rather than an actual low-level wakeup from sleep/stop (the latter will often trigger the former, but not neccesarily).

This is an area of constant rework. The API is modeled more or less after uITRON's "tsk_slp" / "wuk_tsk". Idea is simply that not a "interrupt" condition bails out of sleep/deepsleep, but a event condition. The enableWakeup() / disableWakeup() riffraff might go away. The problem is fundamentally fine control over what constitutes such a event condition. The current model is to statically configure this set (enableWakeup/disableWakeup for defaults, STM32L0.wakeup() for interrupt handlers and callbacks). It might be better to just pass in (optionally) a set/bitmask of events you are interested in to wakeup from. Current problem I am looking at is as to how big such a mask needed to be. RSX11M-Plus ended up doing something similar, but they ended up using 96 bits ...

  • STM32L0.deepsleep() seems to do the same as sleep() (that is, it uses STM32L0_SYSTEM_POLICY_DEEPSLEEP, but AFAICS that is handled identical to STM32L0_SYSTEM_POLICY_SLEEP by stm32l0_system_sleep()?

That is bad naming. Also something I switch forth and back on. "deepsleep" == STOP. POLICY_RUN (used by "delay()") is simple __WFE() without disabling interrupts, while POLICY_SLEEP is SLEEP via __WFI() and bus downclocking. On internal revisions of the core deepsleep() had been replaced again by STM32L0.stop().

@matthijskooijman
Copy link

Thanks for your quick and detailed response :-)

That was intentional. A USB_VBUS pin is required going forward (or for STM32L4/L4+/WB detection via PVM1).

Ok, but what's the reason for this? It seems that the old behavior of assuming VBUS present if there is no pin is reasonable (I even have a commit ready that I wanted to submit a PR for after some more testing). This is also what the USB hardware in STM32F4 implements (that has a VBUS pin for bus voltage detection, but can be disabled to assume VBUS is always enabled). Of course, it is better to have a VBUS detection pin, but that's not always an option (on existing boards, at least).

Naming is always tricky. The USBDevice.attach() came in with the ArduinoCore-samd heritage. However this conflicts with the conventions that the USB spec uses, where "attach" means that the physical connection is established (connector plugged in, VBUS is present).

Hm, I was under the impression that "Attached" was equivalent to "Pullup enabled" in USB standard terminology, but looking through the USB1.1 and 2.0 spec documents, that does not seem to be the case (though I could not even find any mention of the practice of disabling the pullup to force re-enumeration, only some mentions that the pullup must be powered indirectly from VBUS, so it must be disabled when VBUS is gone). Not sure where I got this idea, then.

If you have a better suggestion of how to name "USBDevice.attach()" I am all ears. This function really attaches the USB stack to the connector so to speak.

Yeah, so maybe attach() is acceptable (even if not 100% correct in USB standard terms), especially if it is compatible with the SAMD core? And then to remove confusion, attached() could maybe be renamed to vbus_present() like I suggested (or powered() might be even better, corresponding to the USB spec device state "Powered"). Thinking on this a bit more, "attached" is actually not entirely correct either, since a cable might be attached to an USB host, but the port might be powered down, in which case the current attached() would still return false (so powered() returning false would be more appropriate).

"connected" should be obvious. It means that the peer (host) has established a connection (bus reset issued, address configured). That is important, because it's possible to have a battery connected via USB. Guess I should add the proper BCD detection logic and add USBDevice.detected() which returns 0 while nothing is detected, and a non-zero when something is detected, and of course what ...

Right, that makes sense. I guess I was mostly confused by the implementation, but attaching a debugger I see that the suspend and resume interrupts are indeed triggered directly after a (MCU and thus USB) reset, so I guess that's why the state ends up in CONNECTED. Still, this does not really feel right, since now the state is STARTING when the suspend callback fires, does not switch to SUSPENDED (because that only happens when the state is CONNECTED), and then switches to CONNECTED when the resume callback fires. If this is all intentional, then it's fine, but it feels a little bit like the code has a bug that gets masked because a suspend/resume cycle is triggered because the first SOF doesn't arrive immediately.

USB_VBUS detection is per default disabled during STOP mode. That is why this API is there. The reason is that there is additional power draw on this pin if you enable detection. So there needed to be a choice.

Yeah, I totally agree that having this API is useful and essential. Even apart from the power draw, you might not want to wake up your device on VBUS presence as well. My comment was mostly about the naming of the API. setVBUSDetect() suggests that it could maybe enable or disable VBUS detection completely, which is not the case, and provides no hint that it actually configures wakeup behavior (though SLEEP and SLEEP_AND_STOP do provide that hint). Like I said, once I realized how it works, the naming makes sufficient sense, but there might be a little room for improvement (not sure how exactly, though).

This is an area of constant rework. The API is modeled more or less after uITRON's "tsk_slp" / "wuk_tsk". Idea is simply that not a "interrupt" condition bails out of sleep/deepsleep, but a event condition. The enableWakeup() / disableWakeup() riffraff might go away. The problem is fundamentally fine control over what constitutes such a event condition. The current model is to statically configure this set (enableWakeup/disableWakeup for defaults, STM32L0.wakeup() for interrupt handlers and callbacks). It might be better to just pass in (optionally) a set/bitmask of events you are interested in to wakeup from. Current problem I am looking at is as to how big such a mask needed to be. RSX11M-Plus ended up doing something similar, but they ended up using 96 bits ...

I guess that would make sense. It seems you've even implemented this already (using an "APPLICATION" even right now).

Another approach could be to leave this to the user, similar to how the POSIX sleep (and other blocking) functions work (where you specify a sleep interval and the function blocks for that long, but if a signal happens, the sleep returns -EINTR and it is up to the caller to either sleep again, or decide that something relevent happened and sleep should be aborted). Quite a big leap from the current model, though.

That is bad naming. Also something I switch forth and back on. "deepsleep" == STOP.

Ah, I see now. I was switching between 0.0.10 and master, so didn't notice that it was renamed rather than added, my bad.

As for naming: deepsleep() is probably easier for novice users that just want a "deeper sleep", while stop() better matches the datasheet name (but can be a little confusion, since "stop" implies to never continue). A compromise could be to use deepsleep() as the external interface, but use STM32L0_SYSTEM_POLICY_STOP internally, so that anyone who looks at the code quickly sees that deepsleep is implemented as stop mode internally. Or maybe even have a stop() alias for deepsleep().

@GrumpyOldPizza
Copy link
Owner

Thanks for your quick and detailed response :-)

That was intentional. A USB_VBUS pin is required going forward (or for STM32L4/L4+/WB detection via PVM1).

Ok, but what's the reason for this? It seems that the old behavior of assuming VBUS present if there is no pin is reasonable (I even have a commit ready that I wanted to submit a PR for after some more testing). This is also what the USB hardware in STM32F4 implements (that has a VBUS pin for bus voltage detection, but can be disabled to assume VBUS is always enabled). Of course, it is better to have a VBUS detection pin, but that's not always an option (on existing boards, at least).

The reason is BCD. As soon as VBUS is detected, BCD should be started. If the outcome of BCD is that a host is attached, then the USB stack should be started (and the peripheral enabled). If there is no GPIO for VBUS, you do not get a "attached" event, and all you can do is to start BCD. And if that says "nothing on the other end", you wont start the USB stack later again.

STM32F4 ... Not of interest to me honestly. But it uses the same USB_OTG peripheral as far as I recall. That one if you enable it sucks up 4mA. I have not found a way with that one to make use of the builtin VBUS detection without firing up the whole peripheral, so pointless.

There might be a way to put the USB peripheral in lowpower mode (USB_FS has that), and assume a "suspended" state and simply wait for the wakeup (which in essence is the same as a reset event on the bus). Haven't tried that ever though. For STM32WB we are using VDD_USB directly. You can use a LED to drop VBUS from 5V down to 3V3, and via PVM1 you can detect the "attach" event. Hence no additional GPIO wasted. So I am reluctant to invest time there.

This is an area of constant rework. The API is modeled more or less after uITRON's "tsk_slp" / "wuk_tsk". Idea is simply that not a "interrupt" condition bails out of sleep/deepsleep, but a event condition. The enableWakeup() / disableWakeup() riffraff might go away. The problem is fundamentally fine control over what constitutes such a event condition. The current model is to statically configure this set (enableWakeup/disableWakeup for defaults, STM32L0.wakeup() for interrupt handlers and callbacks). It might be better to just pass in (optionally) a set/bitmask of events you are interested in to wakeup from. Current problem I am looking at is as to how big such a mask needed to be. RSX11M-Plus ended up doing something similar, but they ended up using 96 bits ...

I guess that would make sense. It seems you've even implemented this already (using an "APPLICATION" even right now).

Another approach could be to leave this to the user, similar to how the POSIX sleep (and other blocking) functions work (where you specify a sleep interval and the function blocks for that long, but if a signal happens, the sleep returns -EINTR and it is up to the caller to either sleep again, or decide that something relevent happened and sleep should be aborted). Quite a big leap from the current model, though.

Not really a big leap. STM32L0.sleep() just needed to return say a bool. "true" says "I timed out", and "false" says "I got interrupted". "interruped" as "seen a STM32L0.wakeup() either direct or implied".

It's not quite the same as POSIX there. A signal is something you attach code to. Here if you attach code to something, you don't want to wakeup. You want to let the code you attached to decide whether to wake up or not

I guess I have to explain where all of this is coming from. First off I do not believe that it is a wise idea to expose a RTOS at user level for many, many reasons. So a lot of the standard techniques that imply a RTOS (or POSIX) are not applicable.

You typically have cases like SYSTICK, which often fires in 1ms intervals. STM32L0.sleep() should not return from just any ISR like this firing. Really it should only bail out if user observable state changes. So like Serial1 receiving data. With "attachInterrupt()" things get more complicated. Suppose you have a GPIO that triggers a async I2C read from a sensor. In reality you want to bail out of STM32L0.sleep() only when the sensor data is ready, not when the GPIO triggers the ISR. Hence the need for SM32L0.wakeup(). The real crux is now "user observable state". So you have your USBDevice. Are you really always interested when the USB peripheral goes into suspended mode ? Most applications do not really ever care. So you need somehow a way to say what user observable state you are interested in. One way (which is what I have implemented right now) is to say per peripheral (Serial1, USBDevice), whether the user observable state matters or not (which is the enableWakeup()/disableWakeup() API set). That model makes sense, because the default is to look at all user observable state, but if an application wants, it can remove from this set. The downside is that it's a fairly static, and a fairly obscure model. The alternative is to model all of this instead of using POSIX sleep() after POSIX select(). There you define the set of events (aehm file descriptors, whether it's a read/write/exception condition) that you are interested in.

Anyway, food for thought.

Other folks seem to have run into that very issue as well, not fully understanding ... and with crummy workarounds. Look at ArduinoBLE and HCITransport.wait() ... There the really would like to power down till a HCI event arrives ...

As for naming: deepsleep() is probably easier for novice users that just want a "deeper sleep", while stop() better matches the datasheet name (but can be a little confusion, since "stop" implies to never continue). A compromise could be to use deepsleep() as the external interface, but use STM32L0_SYSTEM_POLICY_STOP internally, so that anyone who looks at the code quickly sees that deepsleep is implemented as stop mode internally. Or maybe even have a stop() alias for deepsleep().

I am back in STM32WB calling this STM32WB.stop(). Ever chip vendor under the sun uses different names. Some say "deepsleep" and mean what ST calls STANDBY. Some use the same name to describe ST's STOP mode. So it's confusing either way. My original intention to use "deepsleep" was to get away from STOP as the code really cycles throu RUN/SLEEP/STOP as needed. If a peripheral is active (like I2C transmitting) in the background and would block STOP mode, then the code will stay in SLEEP for a while till it can enter STOP. And Arduino-LowPower does not make this any easier, so some implementations do it one way "deepsleep" as STOP and some use STANDBY for "deepsleep" ... Even worse some implementation (SAMD) kill SYSTICK (their timebase for millis() and micros()) in ANY mode, while some others ... You get the idea there. It seems cleaner to put this into a STM32L0 class and name it after the specific vendor's nomenclatura and be done. There does not seem to be any "right" or "good" way ...

@matthijskooijman
Copy link

The reason is BCD

Forgive my ignorance, but what is BCD in this context?

I also suspect that, given I haven't heard of it, I won't be needing BCD and the same might hold for others, so if not having VBUS would mean that USB works, but BCD does not, I think I can live with that.

There might be a way to put the USB peripheral in lowpower mode (USB_FS has that), and assume a "suspended" state and simply wait for the wakeup (which in essence is the same as a reset event on the bus).

Sounds like a neat trick that I probably don't really need, but might try anyway :-)

A signal is something you attach code to. Here if you attach code to something, you don't want to wakeup. You want to let the code you attached to decide whether to wake up or not

Yeah, but in POSIX that means that your signal handler would set some value, that makes the main code decide whether to retry (which is usually the default) or abort on -EINTR. A similar thing could be implemented here, but it would make sketches more complicated since they need to implement retries (which are now essentially implemented in stm32l0_system_sleep). But it was just a thought for context, not necessarily an actual suggestion.

It seems cleaner to put this into a STM32L0 class and name it after the specific vendor's nomenclatura and be done.

Agreed, sleeping is going to involve platform-specific knowledge in any case, I'm afraid.

@matthijskooijman
Copy link

It seems that bdd33df removed support for not having an VBUS pin?

Since I really needed this support (we built two dozen prototype boards without that VBUS pin), I implemented that a while ago and now that that code ran fine for a while, I put it into a PR: #175. Let's continue discussion on this subject, what BCD is and if and why it should block supporting boards without a VBUS pin in that PR rather than here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants