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

Improved WS2812B support, fixed library import from 2 different trees, added demo to control neopixels with on/off and brightness control from tux touch pads #32

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

Conversation

marcmerlin
Copy link
Collaborator

No description provided.

led.py:
- had outdated import code, fixed comments
- import configuration.main or
  File "<stdin>", line 1, in <module>
  File "examples/badge_np.py", line 41, in run
  File "/lib/aiko/led.py", line 142, in initialise
AttributeError: 'module' object has no attribute 'main'

- added length_get needed by demoe code
- added write needed by demo code
@marcmerlin
Copy link
Collaborator Author

demo: https://youtu.be/hJYvMYOp11o

@marcmerlin marcmerlin changed the title Allow pushing config file changes easily. Improved WS2812B support and added demo. Jan 24, 2021
@marcmerlin marcmerlin changed the title Improved WS2812B support and added demo. WIP: Improved WS2812B support and added demo. Jan 25, 2021
Copy link

@garthk garthk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay demos!

lib/aiko/led.py Outdated
Comment on lines 5 to 7
# import aiko.led
# aiko.led.initialise()
# aiko.led.pixel(aiko.led.red, 0, True)
# from lib.aiko import led
# led.initialise()
# led.pixel(aiko.led.red, 0, True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda fascinated… aiko.led is imported and initialised by main.py, so we could kinda eliminate those lines; what do we get from the switch from aiko.led.pixel to led.pixel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes and no, I ran this code from repl where it was not visible. There are some weird visibility issues between what's visible from repl and from code run by main.py.
I've had a lot of issues with led.dim being a different value when set from MQTT, or from code. I'm still trying to figure out why, it's very perplexing.
If you are experienced with micropython namespace issues, let me know and I'll share more details.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely clear on it either, it strikes me. I'd swear aiko is always imported on my REPL. Perhaps that's more to do the with limits of the soft reset inflicted by Control-D, though. (I've also noticed poor or zero reliability of MQTT hook-up after ^D, which I need to nut out.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, clear now: we've got the double import problem (see PR-level convo here) and when main.py runs it's doing so with the same globals as the REPL. So, straight after booting you can see aiko, application, common, configuration, plugins, and a bunch of others even though you didn't import them. ^D kicks it all off again.

lib/aiko/led.py Outdated
Comment on lines 139 to 141
def length_get():
return length

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aiko.led.length works… sure, it's not at all encapsulated, but bytes are scarce, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this was also to try namespace stuff, but this one is entirely useless, I removed it in my version.

@marcmerlin
Copy link
Collaborator Author

I'll push my updated version. If you have this working, please let me know if you can figure out why setting up dim values from MQTT, affects the brightness of leds also toggled via MQTT, but not when the LEDs are set by led.pixel(color, i, True)

led_py: Allow setting dimming from code and reading it back
fill by default updates the strip, that's likely what's expected
length_get was a test, removed
added led:debug to see why dim has a different value from MQTT than when
set by code

badge_np:
- added threading support so that it can be run from the main loop
as an application
- added color chase
- added touch pad support (right tux)
- Added toggling left tux LED form code
@marcmerlin
Copy link
Collaborator Author

@garthk please have a look at the latest version I just pushed.
I need to find out why set title isn't working (thankfully print over serial does), and more surprisingly why I get a different value for dim whether I set it from

I was discussing this with @geekscape : my current code outputs this:
loop3: 0.1
Dim: 0.1
Dim: 0.6
Dim: 0.1
loop1: 0.1

the middle 'dim 0.6' happens because I sent this MQTT:
sauron:# mosquitto_pub -h 101.181.46.180 -t public/esp32_X/0/in -m "(led:dim 0.6)"
sauron:
# mosquitto_pub -h 101.181.46.180 -t public/esp32_X/0/in -m "(led:debug)"

so, dim has 2 values depending on where it's set from.

Interesting repl sees the version that is set by MQTT:

from lib.aiko import led; locals()["aiko"].led.dim;globals()["aiko"].led.dim;led.print_dim()
0.6
0.6
Dim: 0.1

Does this make sense to you Garth?

dim global variable in led.py was configured to have a separate
value in the aiko net MQTT thread, and the main thread.
This means that example code calling lib/led.py to set pixels,
got a different dimming value than the same pixels set by the same
led.py when they were set from the MQTT thread.

Workaround is to store the dim value in configuration/main.py which
is imported in memory before the threads happen, and that namespace
is effectively shared between the different threads, so it can be
used to share data.
Using configuration/led.py would not work as it is not imported
in memeory before aiko forks off the net/MQTT thread.
@marcmerlin
Copy link
Collaborator Author

ok, I managed to fix the namespace problem I could tell was there the entire time
@geekscape like I was smelling early on, the MQTT (aiko/net) thread imports aiko/lib/led.py and gets a different version than my code which also imports aiko/lib/net.py from the main thread.
As a result, both threads have a different version of the module with a different value of the dim global variable.
It is a micropython bug? I don't know. I'm assuming that threads that import modules after the thread was created, do not share the same memory and therefore values than if the import had been done before the thread (like is done for configuration/main.py)
See also the comment in c7ccdc0

@marcmerlin
Copy link
Collaborator Author

marcmerlin commented Jan 28, 2021

OMG, I couldn't get setting the title working either and I found out that

from lib.aiko import led,oled

is not the same as

from aiko import led,oled

They both work, but the first one creates a different namespace for all global variables, while he 2nd one does not and the global variables are shared between threads.
Is that an obscure bug, or WAI?

@geekscape I should add that the default files use both interchangeably, when mixing them, in fact introduces this very unclear bug I hit:
lib/aiko/led.py says: # from lib.aiko import led
lib/aiko/oled.py says: # import aiko.oled

Mixing both causes 2 different namespaces and the global variables
aren't shared between access methods and background methods that
actually update the hardware, causing massive confusion.

Now that the very weird bug is found, no need to store 'dim' in
configuration.main.dim

Also update oled.py to fix the import and usage comments.
@garthk
Copy link

garthk commented Jan 28, 2021

Oh, of course. sys.path is ['', '/lib']. You're getting literally two copies of the same module: one loaded via 'lib', the other via ''. For any module with code, it's being run twice.

@garthk
Copy link

garthk commented Jan 28, 2021

Ha! Saved myself a bunch of time explaining it: it's already described as the double import trap in an article explaining how Python module resolution works.

If we remove '' from sys.path, we won't be able to reach applications, configuration, examples, plugins et al nor the built in or frozen modules. If we remove 'lib' it'll be messy… I think it's going to be easiest just not ever importing beginning with lib.

@marcmerlin
Copy link
Collaborator Author

Yes, thanks for confirming what too me way too long to find out. I can see sys.path being helpful, but here is made things much much worse. Even Andy used both forms, causing an unsafe mix, and ultimately bugs like the ones I ended up hitting.
Oh well, despite the price, I learned something about python :)

Front pad: brighter
Rear pad: dimmer
both pads: toggle on/off

led.py: added reset_dim and change_dim.
@marcmerlin marcmerlin changed the title WIP: Improved WS2812B support and added demo. Improved WS2812B support, fixed library import from 2 different trees, added demo to control neopixels with on/off and brightness control from tux touch pads Jan 29, 2021
@marcmerlin
Copy link
Collaborator Author

@garthk I think I fixed all the issues, it works as intended now.
Demo; https://www.youtube.com/watch?v=sNQQ8SSOUU0

Would you mind trying it out to see if it works for you?

@geekscape the code should be good for review and merge if it looks good to you

Copy link

@garthk garthk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't gone over the OLED stuff yet.

configuration/led.py Outdated Show resolved Hide resolved
lib/aiko/led.py Show resolved Hide resolved
lib/aiko/led.py Outdated Show resolved Hide resolved
lib/aiko/led.py Show resolved Hide resolved
lib/aiko/led.py Show resolved Hide resolved
lib/aiko/led.py Outdated Show resolved Hide resolved
Copy link

@garthk garthk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to stop bike-shedding, now. :)

@marcmerlin
Copy link
Collaborator Author

Thanks for the review. Let's now see what @geekscape says since I think he wants to personally review stuff like that before it goes in :)

@marcmerlin
Copy link
Collaborator Author

Hi @geekscape , how's life? :)

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.

2 participants