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

[Feature] Make reset and reset_reason consistent #379

Closed
laurensvalk opened this issue Jul 1, 2021 · 7 comments
Closed

[Feature] Make reset and reset_reason consistent #379

laurensvalk opened this issue Jul 1, 2021 · 7 comments
Labels
enhancement New feature or request software: pybricks-micropython Issues with Pybricks MicroPython firmware (or EV3 runtime)

Comments

@laurensvalk
Copy link
Member

laurensvalk commented Jul 1, 2021

Is your feature request related to a problem? Please describe.
I'm currently documenting the reset functions, but it is a bit hard to explain.

Right now, reset(1) does not reset, and reset() and reset_reason() are the exact opposites:

reset(0) ---> reset_reason() == 1
reset(1) ---> reset_reason() == 0

Is there a reason for this?

Since we're not introducing enums for these, it would make sense to use the same numbers. And if we have to pick one, I'd say "reset(1)" means "reset", so:

reset(1) ---> reset_reason() == 1 // This is reset
reset(0) ---> reset_reason() == 0 // This is shutdown

EDIT: Pushed proposed change.

@laurensvalk laurensvalk added enhancement New feature or request software: pybricks-micropython Issues with Pybricks MicroPython firmware (or EV3 runtime) labels Jul 1, 2021
laurensvalk added a commit to pybricks/pybricks-micropython that referenced this issue Jul 1, 2021
pybricks/support#379

Even though these are not the same enums, it makes sense to align the first two entries of both as they can be used complementary.
laurensvalk added a commit to pybricks/pybricks-micropython that referenced this issue Jul 1, 2021
pybricks/support#379

Even though these are not the same enums, it makes sense to align the first two entries of both as they can be used complementary.
@dlech
Copy link
Member

dlech commented Jul 1, 2021

We've already released v3.0, so this would be a breaking change. If we wanted to use the reset function as part of the firmware upgrade process, it would make things more complicated since the value we would have to pass would depend on the current firmware version.

@laurensvalk
Copy link
Member Author

reset(2) wouldn't change though.

@dlech
Copy link
Member

dlech commented Jul 1, 2021

I chose 0 for reset since that is "default" operation. The reset reasons were chosen to match MicroPython's machine.reset_cause() but I see now that I was off by 1.

@dlech
Copy link
Member

dlech commented Jul 1, 2021

Independent of the numbering, here are some suggestions for the docs:

Reset:

0: Reboots the hub (hard reset)
1: Powers off the hub without proper shutdown (don't use this)
2: Reboots the hub and places it in firmware update mode (not supported on all hubs)

Reset reason:

0: The hub booted directly to Pybricks firmware from a powered off state.
1: The hub was reset by software (this might by seen after a firmware update depending on the bootloader)
2: The hub was reset by the watchdog timer because the runtime stopped. Ideally, this shouldn't happen, so please report a bug if you do see this.

@dlech
Copy link
Member

dlech commented Jul 1, 2021

And we should be wary of the MicroPython definitions of hard and soft reset. Hard reset != hardware reset (not strictly anyway). Soft reset != software reset. Hard reset == (software [or hardware? - not applicable to Pybricks anyway since we don't have a reset button]) reset of the MCU. Soft reset == reset of the MicroPython runtime without resetting the MCU.

@laurensvalk
Copy link
Member Author

Given

  1. (...) don't use this
  2. (...) not supported on all hubs

and,

We need to add a new method for proper shutdown

it sounds like maybe the existing reset() method shouldn't be in the documented end user API? We can still just keep it in the implementation as-is for backwards compatibility and/or system updates.

If we do, we can also just keep and document reset_reason as-is, since then its arguments won't be as confusing.

And we should be wary of the MicroPython definitions of hard and soft reset. Hard reset != hardware reset (not strictly anyway). Soft reset != software reset. Hard reset == (software [or hardware? - not applicable to Pybricks anyway since we don't have a reset button]) reset of the MCU. Soft reset == reset of the MicroPython runtime without resetting the MCU.

Since we're not aiming to recreate umachine, we might not need all this detail. (Here's a draft I wrote prior to this discussion.)

Whether it's two methods or just one with an argument, I think all we'd need for end users are safe/usable variants of these:

  • Shut down
  • Reboot

I'm looking at these terms from the end-user perspective, even if they don't truly power off in a technical sense. Hence this wording (as opposed to power off and reset).

Shut down is mainly useful for long-running creations that you may want to turn off on some condition. Such as after N hours, on completing a complicated drawing, on reaching the charging-pad, etc.

EDIT: We might not even need any form of reboot, especially if it's causing more trouble than it's worth, like #382. Unlike many MicroPython boards, a lot is already cleaned up by just exiting the script anyway. If the user needs to reset other things, like maybe ports or Bluetooth connections, it might be better to use dedicated methods instead of rebooting.

laurensvalk added a commit to pybricks/pybricks-micropython that referenced this issue Jul 5, 2021
Reset will not be part of the end-user API. We will only keep it for backwards-compatibility for system updates.

There will be a graceful shutdown instead. This adds the placeholder and moves the corresponding TODO note.

pybricks/support#379
laurensvalk added a commit to pybricks/pybricks-micropython that referenced this issue Jul 5, 2021
hub.system.reset(2) is kept in the implementation for backwards-compatibility with system updates, but the other reset variants should not be used, since they can lead to problems like pybricks/support#381

pybricks/support#379
@laurensvalk
Copy link
Member Author

Resolved via Skype. We are going for the implementation as sketched above.

tl;dr:

  • reset(action) will not be part of the user-api but its implementation will partially stay for backwards compatibility for firmware updates
  • shutdown() will be added and documented instead, providing grafeful shutdown, which is not always power-off.
  • reset_reason() will be documented as-is, with numbering unchanged.

laurensvalk added a commit to pybricks/pybricks-api that referenced this issue Jul 6, 2021
Also drop reset() and update reset_reason() after pybricks/support#379
laurensvalk added a commit to pybricks/pybricks-api that referenced this issue Jul 7, 2021
Also drop reset() and update reset_reason() after pybricks/support#379
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request software: pybricks-micropython Issues with Pybricks MicroPython firmware (or EV3 runtime)
Projects
None yet
Development

No branches or pull requests

2 participants