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

Power off improvements #265

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

dlech
Copy link
Member

@dlech dlech commented Aug 31, 2024

This is an attempt to do a better job shutting off all I/O port devices when powering off the hub.

The last commit fixes the issue identified in https://github.com/orgs/pybricks/discussions/1795#discussioncomment-10507805.

The middle commit fixes sensors like the BOOST Color/Distance sensor staying on while the button is held down at power off.

The first commit is setting up for the second commit.

@dlech
Copy link
Member Author

dlech commented Aug 31, 2024

I tested briefly on City Hub and SPIKE Prime, but this could use some more @BertLindeman level QA testing ™️ 😄

@coveralls
Copy link

coveralls commented Aug 31, 2024

Coverage Status

coverage: 56.033% (-0.009%) from 56.042%
when pulling 91c5940 on dlech:power-off-improvements
into bd7c345 on pybricks:master.

@BertLindeman
Copy link
Contributor

BertLindeman commented Aug 31, 2024

Tested movehub at build 3514. Color/distance. Press button and keep pressed. Hub goes off and sensor re-lights again.
Cityhub at 3514 does the same. Could not trigger this on spike prime / Robot Inventor / Technic hub.

[EDIT] added Technic hub. Going to test with build 3517

City hub no regression seen on issue 385
no re-power-on on the Color-Distance sensor.

Movehub same positive result.

Technichub same positive result.

A-Oh Regression:
Spike at build 3517 with USB connected after power-off does light the Color sensor and the Color-distance sensor again.
And also with the program for issue 385 the error is back:

from pybricks.hubs import ThisHub
from pybricks.tools import wait

hub = ThisHub()

print(hub.system.reset_reason())

wait(100)

hub.system.shutdown()

The hub mimics "I am powered off" but the Color sensor and the Color-distance sensor flip-flop off and on at ~ 1 Hz?

@BertLindeman
Copy link
Contributor

BertLindeman commented Aug 31, 2024

David as I do not know if you see updated comments, so I add this comment to let you know I finished this tests.

Bert

{EDIT] movie of Spike prime hub power-off while USB connected to loader:

2024-08-31.23.40.14_480.mp4

@dlech
Copy link
Member Author

dlech commented Sep 2, 2024

Hi @BertLindeman, thanks for all the testing!

Press button and keep pressed. Hub goes off and sensor re-lights again.

I'm not able to reproduce this. Here is what I am doing:

  • Attach sensor to City hub or Move hub
  • Press and release button to turn on hub
  • Press and hold button to turn off hub
  • Keep holding button for 10 sec to see if issue reproduces
  • All lights stay off
  • Release button

Spike at build 3517 with USB connected after power-off does light the Color sensor and the Color-distance sensor again.

I am able to reproduce this issue.

And also with the program for issue 385 the error is back:

Which error are you referring to here?

@BertLindeman
Copy link
Contributor

And also with the program for issue 385 the error is back:

Which error are you referring to here?

What I referred to was the Spike hub. As you reproduced.

Not the city hub.

Sorry for the confusion. Issue 365 is not back.

@dlech
Copy link
Member Author

dlech commented Sep 2, 2024

Spike at build 3517 with USB connected after power-off does light the Color sensor and the Color-distance sensor again.

This issue should be fixed now (build 3526).

@dlech
Copy link
Member Author

dlech commented Sep 2, 2024

Press button and keep pressed. Hub goes off and sensor re-lights again.

I'm not able to reproduce this. Here is what I am doing:

Ah, I think you were referring to a build from before this pull request. So all good now?

@BertLindeman
Copy link
Contributor

Test-mode ON
All at build 3526
run the issue_385 program and verify no re-power-on

  • Spike-prime USB connected - OK
  • technichub - OK
  • cityhub - OK
  • movehub - OK

Long press power button if hub is ON and keep pressed and verify color sensor and color-distance sensor stay OFF
(No spike 3x3 matrix available here)

  • Spike-prime USB connected - OK
  • technichub - OK
  • cityhub - OK
  • movehub - OK

OK for me, David.

@BertLindeman
Copy link
Contributor

David,
Ran build 3527.

scenario:
spike prime or Robot Inventor:

  • run issue_385 program (see some post before this) with USB connected to load the battery
  • run the program (F5), the hub does a shutdown, all OK
  • set the hub on again and run the program by a button press. (Still USB connected!)
    Now the Bluetooth button stays blue.
    This occurs also on build 3514 (I just happened to have available)

Long press to power-off still does put off all lights.

No idea how come, Laurens did not implement the light refactor, did he?

Bert

@dlech
Copy link
Member Author

dlech commented Sep 3, 2024

No idea how come, Laurens did not implement the light refactor, did he?

There were some changes to the Bluetooth button merged already, so that is probably a side effect of those changes. So hopefully that will be fixed by one of the other open pull requests.

@dlech
Copy link
Member Author

dlech commented Sep 3, 2024

Reported on https://github.com/orgs/pybricks/discussions/1801, MoveHub still need work.

@laurensvalk
Copy link
Member

Thanks for the pull request!

Is it possible to make a short list of hub/sensor/usb poweroff permutations that this now fixes?

staying on while the button is held down at power off.

Is this the only issue or does something also stay on permanently when connected to USB?

@dlech
Copy link
Member Author

dlech commented Sep 3, 2024

Is this the only issue or does something also stay on permanently when connected to USB?

USB connected and power button pressed both keep the power on, so there is really no difference between those. If something stays on when USB is connected, then it will also stay on if the power button is pressed.

Is it possible to make a short list of hub/sensor/usb poweroff permutations that this now fixes?

I don't know all of the sensors that request power on P1 or P2, but these are affected. Before, power on P1 or P2 was left on, now it is turned off. This was only noticeable without a volt meter on the SPIKE color light matrix since it turned on all green because of this.

Before this, P4 was powered off when shutdown was requested (it doesn't matter if a device is connected or not). After this change, they don't power off until shutdown is complete. So now, the hub status light and the sensors turn off at the same time. Before, the sensors would turn off, then the hub status light would blink after that. The difference is only noticeable to the user if the sensor has a light.

Bug that still needs to be fixed. If Move hub has a sensor attached, all works as expected. But if Move hub doesn't have a sensor attached, it will turn itself back on after the button is released. This bug was most likely introduced by these changes.

@laurensvalk
Copy link
Member

Thanks for the overview David.

@BertLindeman -

In short: if the hub is not powered-off the power-led color is remembered or not reset.

Coincidentally, this was fixed in 6d65df1 earlier today.

@BertLindeman
Copy link
Contributor

BertLindeman commented Sep 5, 2024

[EDIT] For clarity add a column to the table with the build 3537 result.

Power button press patterns.

  1. Long power button press and release on power-off as soon as blinking stops

  2. Long power button press and release after (half) a second? after blinking stops
    This is a tricky measurement. How long is not so long?
    Have seen observations where a pattern 2 test kept the hub off and later on again, with color-distance sensor on port-D

  3. Long power button press and keep a few seconds (3 – 5)

Tests on movehub at build 3527 and build 3537

description attachment(s) result 3527 result 3537
pattern 1 -none- hub switches ON hub stays OFF – OK
pattern 1 LED lights on C hub switches ON hub stays OFF – OK
pattern 1 Color sensor on C hub stays OFF – OK hub stays OFF – OK
pattern 1 Color/DIST sensor on C hub switches ON hub stays OFF – OK
pattern 2 -none- hub switches ON hub stays OFF – OK
pattern 2 LED lights on C hub stays OFF – OK hub stays OFF – OK
pattern 2 Color sensor on C hub stays OFF – OK hub stays OFF – OK
pattern 2 Color/DIST sensor on C hub switches ON hub stays OFF – OK
pattern 2 * Color/DIST sensor on D hub stays OFF sometimes !! hub stays OFF – OK
pattern 3 -none- hub switches ON hub switches ON
pattern 3 LED lights on C hub stays OFF – OK hub stays OFF – OK
pattern 3 Color sensor on C hub stays OFF – OK hub switches ON
pattern 3 Color/DIST sensor on C hub switches ON hub switches ON

Conclusion

If a color-sensor is connected to port-C all is OK, even if a color-distance sensor is on Port.D.
If a LED-light is attached on Port-C with either nothing or a Color-Distance sensor
on Port-D pattern 1 causes a hub switch ON again.

Repeat this on the current build, and added to the table.

Bert

Up to now, the VCC power off was done when shutdown was requested. This
will cause external sensors to power off. Users may see this and assume
that the hub is now off. However, the hub is still busy with the
shutdown process, e.g. writing to flash memory. So we don't want the
user to release the power button yet.

Instead, move the VCC power off to after the hub has finished everything
and it is safe to power off.

This lets us remove the sys/io_ports.c and sys/io_ports.h files. The
function is renamed to pbdrv_ioport_power_off() to be consistent with
pbdrv_reset_power_off().
Add a new PBDRV_CONFIG_IOPORT_PUP_QUIRK_SHUTDOWN configuration flag for
the ioport_pup driver. This flag is used to indicate that the driver
that special handling of the I/O port VCC pin is needed during shutdown.

We already had such a quirk in place, but it was enabled at all times
on all hubs. This improves the situation by only enabling the quirk on
hubs that need it. Also, on hubs that needed it, we now only enable the
quirk when the button is not pressed. This has the desired effect of
turning off sensors before the user released the button so that it looks
like the hub is really off. This way it will be more clear to the user
that they can release the button because the hub is "off".
In most cases, pbio_stop_all() was already stopping motors, but it
doesn't turn off motor outputs for non-motor devices that need extra
power. This was causing the SPIKE Light Matrix to turn on all green LEDs
when any hub was turned off.

This fix is a bit of a hack since it is going behind the back of the
higher-level I/O port stuff, but it works for now.
@dlech
Copy link
Member Author

dlech commented Sep 7, 2024

OK, hopefully I got this working with all combinations now. Also rebased on current master, so other recent status light/bluetooth light changes should be included.

Amending my previous summary of changes, the most noticeable differences are:

  1. On Move hub and City Hub - before: any sensor with lights would stay on until the button was released on power off - after: the sensor lights turn off at the same time as the hub status light, even while the button is still pressed.
  2. All hubs plus color light matrix - before: the color light matrix would turn all green after a short time when the hub was "off" but still powered due to button pressed or USB connected - after: the color light matrix sensor stays off when the hub is "off", even when the hub is still powered due to button pressed or USB connected.

@BertLindeman
Copy link
Contributor

Just to be sure, David. Will test using build 3542 for this PR

@dlech
Copy link
Member Author

dlech commented Sep 7, 2024

Will test using build 3542 for this PR

Yup, that should be the latest and greatest.

@BertLindeman
Copy link
Contributor

No 3x3 light matrix here, so not tested.

Maybe Manfred @msw-home can test?
Related to discussion 1795 LED Matrix schaltet sich eigenständig wieder ein

Tested cityhub, movehub like the table above. All OK
Similar with technichub and primehub. OK too.

Great David.

Bert

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

Successfully merging this pull request may close these issues.

4 participants