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

Improvement to Badger 2040 operation on battery #6208

Merged
merged 4 commits into from
Mar 30, 2022

Conversation

ZodiusInfuser
Copy link

Badger2040 has a 3V3 enable like that it can set high to keep itself awake, or set low to turn off. The 5 front buttons wake the board up.

The intention was that the user would press a button, then user code would kick in to enable the pin to keep the board awake from thereon. However, it seems that CircuitPython takes a while to set itself up, requiring the user to hold one of the front buttons for over 10 seconds.

I have now moved the pin initialisation and enabling into board_init, reducing that time down to around 1 second. Basically after the activity LED stops flashing. I have also exposed the digitalio object in board so the user can turn the board off after their code has run.

Please let me know if this process is okay? The only example I could find of a similar use was:

common_hal_digitalio_digitalinout_construct(&CTR_5V, PIN_CTR_5V);
and they did not expose the pin to the user to modify later.

Also, I wonder if I should do similar for all the user buttons on the product since everyone needs to include the same digitalio setup code anyway? 🤔

@TheKitty
Copy link

ImportError: cannot import name 'get_terminal_size' from 'click.termui'

@ZodiusInfuser
Copy link
Author

ZodiusInfuser commented Mar 28, 2022

Yes, I see. I don't know what the issue there would be, given I do not even import those within my commit. Input would be welcome

@tekktrik
Copy link
Member

tekktrik commented Mar 28, 2022

It's stemming from an update to the click library that cascadetoml uses. The libraries are breaking for the same reason since black also has click as an indirect dependency.

@tekktrik
Copy link
Member

@jepler is working on sorting it out in adafruit/cascadetoml#8

@dhalbert
Copy link
Collaborator

@ZodiusInfuser If you push an empty commit or trivial change, it should now rebuild and work. We fixed a problem that was breaking the builds: #6210.

@ZodiusInfuser
Copy link
Author

Thanks @dhalbert. I've done as you suggested, though it doesn't look like any actions have been triggered.
Also, before this gets approved, any thoughts on my approach for exposing the digitalio as asked in the original message?

@dhalbert
Copy link
Collaborator

It's now failing for a different reason; there's a compile error, for example: https://github.com/adafruit/circuitpython/runs/5735590430?check_suite_focus=true#step:9:159

Re your pin query: In principle, I don't think there's a problem exposing an already-created DigitalInOut the way you have. You could also just initialize the pin the way you want, using the HAL GPIO operations (or the common_hal_digitalio operations) and have the user create the object if they wanted it.

If you do want to do this: since this is a new concept, there could be user confusion about whether you're exposing a Pin or DigitalInOut object, and you might want to pick a affordant name, like ENABLE_DIO, or ENABLE_DIGITALINOUT, to reduce your support load.

I am surprised at your 10-second delay. Have you tried doing this in boot.py rather than code.py? If code.py is large, I guess the compilation might take a while.

@jepler
Copy link
Member

jepler commented Mar 29, 2022

Now the build error is related to your change. Please take a look. I added an additional comment at the spot that may be the cause of this error, but didn't do any additional building or testing. Thanks!

make: Entering directory '/home/runner/work/circuitpython/circuitpython/ports/raspberrypi'
Use make V=1, make V=2 or set BUILD_VERBOSE similarly in your environment to increase build verbosity.
/usr/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build-pimoroni_badger2040/boards/pimoroni_badger2040/pins.o:(.bss.enable_pin_obj+0x0): multiple definition of `enable_pin_obj'; build-pimoroni_badger2040/boards/pimoroni_badger2040/board.o:(.bss.enable_pin_obj+0x0): first defined here
collect2: error: ld returned 1 exit status
make: *** [Makefile:283: build-pimoroni_badger2040/firmware.elf] Error 1
make: Leaving directory '/home/runner/work/circuitpython/circuitpython/ports/raspberrypi'

@ZodiusInfuser
Copy link
Author

Re your pin query: In principle, I don't think there's a problem exposing an already-created DigitalInOut the way you have. You could also just initialize the pin the way you want, using the HAL GPIO operations (or the common_hal_digitalio operations) and have the user create the object if they wanted it.

I tried doing the HAL solution first, but think I encountered issues when the user code created the DigitalInOut object, which would reset the pin back to a default state before enabling, inadvertently shutting the board off. I could be wrong there though

If you do want to do this: since this is a new concept, there could be user confusion about whether you're exposing a Pin or DigitalInOut object, and you might want to pick a affordant name, like ENABLE_DIO, or ENABLE_DIGITALINOUT, to reduce your support load.

Those do sound like better names, I'll switch to one of them.

I am surprised at your 10-second delay. Have you tried doing this in boot.py rather than code.py? If code.py is large, I guess the compilation might take a while.

The setup was within the user code I believe. I will give boot.py a try.

Now the build error is related to your change. Please take a look. I added an additional comment at the spot that may be the cause of this error, but didn't do any additional building or testing. Thanks!

make: Entering directory '/home/runner/work/circuitpython/circuitpython/ports/raspberrypi'
Use make V=1, make V=2 or set BUILD_VERBOSE similarly in your environment to increase build verbosity.
/usr/bin/../lib/gcc/arm-none-eabi/10.2.1/../../../../arm-none-eabi/bin/ld: build-pimoroni_badger2040/boards/pimoroni_badger2040/pins.o:(.bss.enable_pin_obj+0x0): multiple definition of `enable_pin_obj'; build-pimoroni_badger2040/boards/pimoroni_badger2040/board.o:(.bss.enable_pin_obj+0x0): first defined here
collect2: error: ld returned 1 exit status
make: *** [Makefile:283: build-pimoroni_badger2040/firmware.elf] Error 1
make: Leaving directory '/home/runner/work/circuitpython/circuitpython/ports/raspberrypi'

Odd, I haven't encountered that compiler error locally. Would that extern you suggested resolve this?

@dhalbert
Copy link
Collaborator

Odd, I haven't encountered that compiler error locally. Would that extern you suggested resolve this?

I think it should (suggested by @jepler). What compiler version are you using?

@ZodiusInfuser ZodiusInfuser marked this pull request as draft March 30, 2022 17:10
@ZodiusInfuser
Copy link
Author

The extern thing made the CI happy @jepler!

I am surprised at your 10-second delay. Have you tried doing this in boot.py rather than code.py? If code.py is large, I guess the compilation might take a while.

Further to this. It does seem like the 10 second delay was just due to the example I was using. I wrote a simpler example and that kicks in within 2 seconds (not accurately measured), so I tried setting the pin in boot.py as suggested. Unfortunately, for whatever reason either the code didn't execute or the pin was reset between boot.py and code.py, causing the board to shut off upon button release.

This premade DigitalInOut object, although unconventional, offers a good user experience though, so lets go with that.

@ZodiusInfuser ZodiusInfuser marked this pull request as ready for review March 30, 2022 17:33
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for your thoughts. I like this as is now. It's a new idea for default board objects, and that's good, and might be a model for other such things.

@ZodiusInfuser
Copy link
Author

Thanks @dhalbert!

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 4, 2022

@ZodiusInfuser Check the discussion here: #6227, about an issue that came up because board.DISPLAY was an object and not a function like, say board.I2C(). Because board.DISPLAY is only an attribute, it cannot be set to None: the attribute dictionary is in flash and not RAM, to save RAM. It would have been better if board.DISPLAY were board.DISPLAY(). We may eventually make some change that does that one way or another, such as introducing a function, say, board.DISP(), and eventually deprecating board.DISPLAY().

I'm not sure tht board.ENABLE_DIO is comparable. But if you make a function that returns a singleton, board.ENABLE_DIO(), it might be a bit more flexible in the long run, or prevent some initialization or deinitialization problems.

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.

5 participants