pybricks / support

Pybricks support and general discussion
MIT License
109 stars 7 forks source link

[Bug] Sensor's only works once. Hub needs restart to make them work again #456

Closed Vinz1911 closed 3 years ago

Vinz1911 commented 3 years ago

Describe the bug I tried the following on the Inventor Hub with the ColorSensor and the UltraSonic Sensor: Version: ('primehub', '3.1.0a3', 'v3.1.0a3-62-g51bdd097 on 2021-08-26')

Problem: Sensor's only works on first run of the script after stopping the script and run it again, the sensor's are not working. After restart of the hub, the sensor's work again for the first run and then the problem appears again.

To reproduce Steps to reproduce the behavior: Just run this script, stop it and run it again:

from pybricks.pupdevices import ColorSensor, UltrasonicSensor
from pybricks.parameters import Port
from pybricks.tools import wait
from pybricks import version

# Initialize the sensor.
sensor = UltrasonicSensor(Port.C)
sensor.lights.on(100)

while True:
    distance = sensor.distance()
    print(distance)
    wait(100)

Expected behavior Sensor's work every time the script is running

BertLindeman commented 3 years ago

@Vinz1911 I tried this on my cityhub (as it was next to me) with an UltraSonic sensor: What I noticed: First run after hub boot and BT connect running as expected. Stop run and start the program again:

After disconnect and re-boot the hub: the first run is OK.

{edit typo]

Vinz1911 commented 3 years ago

Hi @BertLindeman

Exactly, thats also the behaviour i discovered. With the Color Sensor it shows constantly Color.NONE

BertLindeman commented 3 years ago

@Vinz1911 I used ambient on the color sensor and THAT keep working the second run. My current program:

print(version)
from pybricks.pupdevices import ColorSensor, UltrasonicSensor
from pybricks.parameters import Port
from pybricks.tools import wait
from pybricks import version

# Initialize the sensor.
sensorA = UltrasonicSensor(Port.A)
sensorA.lights.on(60)
sensorB = ColorSensor(Port.B)
sensorB.lights.on(60)
i = 0
while True:
    # distance = sensor.distance()
    # print(distance)
    i += 1
    sensorA.lights.on(i)
    sensorB.lights.on(60)
    print(i, sensorA.distance(), sensorB.ambient())
    wait(500)

Part of the second run:

('cityhub', '3.1.0a3', 'v3.1.0a3-60-ga2b2e44e on 2021-08-18')
1 2000 0.2
2 2000 0.2
3 2000 0.2
4 2000 0.1
5 2000 0.0

[edit: forgot to paste the program]

Vinz1911 commented 3 years ago

Just as Info:

I discovered that you don't need to use the sensor's in Code. You can also run the default main script delivered with the Firmware. Press Start, then Stop will disable the Sensor (Light's Go off) and then they stay off.

laurensvalk commented 3 years ago

Thanks for opening this issue. This used to be fixed, but it sounds like it has come back.

If you want to help, it would be great to know when this started happening by going through older builds.

Which was the last build that worked? It's usually best to make big jumps first (like going back a couple of weeks), and then make smaller steps as you get closer.

I discovered that you don't need to use the sensor's in Code. You can also run the default main script delivered with the Firmware. Press Start, theb Stop will disable the Sensor (Light's Go off) and then they stay off.

This is good, because it's relatively quick to see whether it was still working for a particular version.

The probably happens because we turn off power to all ports at the end of a program. There is a check to skip this for LEGO sensors with particular properties, but apparently this is no longer being checked.

Vinz1911 commented 3 years ago

Hi Laurens,

Yes sure, i will test it later today and give you feedback here 😊

BertLindeman commented 3 years ago

Intermediate result, so we do not do all the same ;-)

firmware from Build #1221 has the error. Version ('technichub', '3.1.0a3', 'v3.1.0a3-60-ga2b2e44e on 2021-08-18')

firmware from Pre release v3.1.0a1 is OK. Version ('technichub', '3.1.0a1', '3ab93ae on 2021-06-23')

firmware from Pre release v3.1.0a3 has the error. Version ('technichub', '3.1.0a3', 'v3.1.0a3 on 2021-07-19')

[edit: will add to this comment as I go along.. will strike through irrelevant versions]

Vinz1911 commented 3 years ago

My Results:

works fine with (also tested all functions of the sensors): ('primehub', '3.1.0a1', '3ab93aef on 2021-06-23')

same with the alpha2 ('primehub', '3.1.0a2', 'v3.1.0a2 on 2021-07-06')

and broke with the alpha3 ('primehub', '3.1.0a3', 'v3.1.0a3 on 2021-07-19')

so anything broke with the alpha3 version, all version before seems to work very well

laurensvalk commented 3 years ago

Thank you both!

Bonus points and eternal gratitude shall be awarded for finding the specific commit in between those releases :smile:

Vinz1911 commented 3 years ago

we will do our best 😜

Vinz1911 commented 3 years ago

Here we go:

latest working one: ('primehub', '3.1.0a2', 'v3.1.0a2-4-gde278587 on 2021-07-07')

and then broke with: ('primehub', '3.1.0a2', 'v3.1.0a2-5-g4d6d1724 on 2021-07-13')

pbio/servo: Make angle reset optional.
Build #1161: Commit 4d6d172 pushed by laurensvalk
laurensvalk commented 3 years ago

Thanks guys! I'll look into this and make sure to fix it.

laurensvalk commented 3 years ago

This is also a good reminder to run our physical tests from time to time. We probably need to document this somewhere.

laurensvalk commented 3 years ago

This used to be fixed, but it sounds like it has come back.

https://github.com/pybricks/pybricks-micropython/commit/baada4898735e87ad5d58b7fc535dc0f6a478671

This caused powered sensors to be reset and not turned back on after a program completed. It would be better to review this logic for the next release instead of breaking the release candidate.

We'll probably have to have a closer look at this so we don't break it once again.

BertLindeman commented 3 years ago

This is also a good reminder to run our physical tests from time to time. We probably need to document this somewhere.

And this issue shows that running a test only once is not enough.

laurensvalk commented 3 years ago

The run_all runs all scripts, so it would have caught this if we actually ran it.

laurensvalk commented 3 years ago

I've found the underlying issue and a workaround. Don't worry too much about understanding the jargon below.

Deep down, the problem is that pbdrv_counter_get_dev succeeds even when there isn't a motor attached. This causes the motor process logic to think there's a motor, so the power is turned off.

This used to be fine, because before https://github.com/pybricks/pybricks-micropython/commit/4d6d172, we always made at least one call to pbio_tacho_get_count during reset, which does fail correctly.

The workaround is to always make that call anyway. We were considering revising device management and pbio more generally, but probably not for the upcoming release. The workaround may have to do, unless we want to update pbdrv_counter_get_dev to return the expected error.

laurensvalk commented 3 years ago

When https://github.com/pybricks/pybricks-micropython/commit/7a24497d8fea56140b7a6ab50f0f72301c392188 finishes building, you can verify if this fixes it.

BertLindeman commented 3 years ago

Can confirm: OK running ('technichub', '3.1.0a4', 'v3.1.0a4-3-g7a24497d on 2021-08-31')

Have problems downloading this firmware (using code.pybricks.com and beta.pybricks.com) to both my cityhub and movehub. So it took more time than I wanted. While downloading the default firmware (just press the install firmware button) goes OK.

Anyway fix works.

Vinz1911 commented 3 years ago

Thank you Laurens,

everything works now (again) es expected 👍

laurensvalk commented 3 years ago

Thanks both of you for reporting and verifying! This is a big help, I appreciate it.

I'll keep the issue open until it's merged to master.