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

NotImplementedError could give reason why not implemented #9798

Open
domints opened this issue Nov 8, 2024 · 10 comments
Open

NotImplementedError could give reason why not implemented #9798

domints opened this issue Nov 8, 2024 · 10 comments
Milestone

Comments

@domints
Copy link

domints commented Nov 8, 2024

CircuitPython version

Adafruit CircuitPython 9.2.0 on 2024-10-28; Adafruit NeoPixel Trinkey M0 with samd21e18

Code/REPL

#import board
#import touchio
#import neopixel
#import time
#import storage
import supervisor
#import usb_cdc

#pixel_pin = board.NEOPIXEL
#num_pixels = 4
#top_touch = touchio.TouchIn(board.TOUCH1)
#bot_touch = touchio.TouchIn(board.TOUCH2)
#pixels = neopixel.NeoPixel(pixel_pin, num_pixels, brightness=0.01, auto_write=True)

supervisor.set_usb_identification('Logitech, Inc.', 'Unifying Receiver', 1133, 50475)

Behavior

boot.py output:
Traceback (most recent call last):
  File "boot.py", line 15, in <module>
NotImplementedError: 

Description

I wanted my Trinkey to pretend it's Unifying Receiver, so I used set_usb_identification function as per documentation.

Documentation says:

This method must be called in boot.py to have any effect.
Not available on boards without native USB support.

which, I believe, both cases are fulfilled - I call it in boot.py, and as far as I know samd21e18 has native hardware USB implementation.
Maybe CircuitPython uses some hacky USB stack for this MCU, digispark-like software instead of native hardware peripheral?

Additional information

No response

@domints domints added the bug label Nov 8, 2024
@dhalbert
Copy link
Collaborator

dhalbert commented Nov 8, 2024

It's because space is extremely tight on the Trinkeys. When this feature was added it overflowed builds on the smallest boards. See #6247 for a little discussion on this. It would be possible to make a custom build to make it fit. It might fit in some language translations.

@dhalbert dhalbert added this to the Support milestone Nov 8, 2024
@dhalbert dhalbert added enhancement and removed bug labels Nov 8, 2024
@domints
Copy link
Author

domints commented Nov 11, 2024

It looks like it was CircuitPython 7.3, and now we have 9.2 and maybe it's more lightweight? :D

I ran make -j4 BOARD=neopixel_trinkey_m0 CIRCUITPY_USB_IDENTIFICATION=1 (I was experimenting with hard-coding the VID/PID pair I wanted in the first place) and it seems that now this piece of code fits:
obraz
yup, it uses 336b more than image built with flags as is, but maybe if there are other limitations to Trinkey, they can be taken back? I have yet to test this version with CIRCUITPY_USB_IDENTIFICATION flag :)

P.S. bare build for this Trinkey gives uf2 that has exactly the same size as the one from the website (364032 bytes) so I assume I didn't screw anything up :D

@domints
Copy link
Author

domints commented Nov 11, 2024

Yup, works :D
So let's say I'll stay on this build for the time being :D
obraz
Do you recall any list of features that might be disabled in Trinkey because of lacking memory, other than supervisor.set_usb_identification?
CIRCUITPY_ONEWIREIO fits, although I am not sure it's that crucial on board that doesn't have GPIO layed out :D
CIRCUITPY_SAFEMODE_PY also fits?

@dhalbert
Copy link
Collaborator

dhalbert commented Nov 11, 2024

Try TRANSLATION=ja, or de_DE, or fr. Those are larger translations that may overflow. You need to do a make clean when switching translations.

@domints
Copy link
Author

domints commented Nov 11, 2024

make clean didn't do much for me:
obraz
However with command line:

rm -rf build-neopixel_trinkey_m0 && make clean && make -j8 BOARD=neopixel_trinkey_m0 CIRCUITPY_USB_IDENTIFICATION=1 CIRCUITPY_ONEWIREIO=1 CIRCUITPY_SAFEMODE_PY=1 TRANSLATION=ja

Japanese built correctly:
obraz
German went fine:
obraz
French is cool:
obraz
Also, zh_Latn_pinyin seems big in a locale folder, but isn't any bigger than French when built into firmware :)

@dhalbert
Copy link
Collaborator

You have to do make clean BOARD=....

Try submitting a PR with CIRCUITPY_USB_IDENTIFICATION turned on for the board, or for all SAMD21 boards. That will run the biggest translations and we can see whether it fits. However, by turning this on we use up space that we might need for something else more important in the future, so I'm not sure we want to do that. It's a balancing act.

It might fit for this particular Trinkey but not others.

Now that you know how to do builds, you can make your own custom build here.

@domints
Copy link
Author

domints commented Nov 11, 2024

Sure, I can build it myself, even can hardcode VID/PID etc myself, no problem. My issue was that it had this weird behaviour, while there was no docs saying that I should expect this behaviour, so after you pointed towards the PR I started going deeper into the rabbit hole :)

It's just that having this single piece of API unavailable for some reason is counter-intuitive. Maybe it could return message indicating why it's saying NotImplementedException?

Also, if I have some free time I'll try, maybe my Github Actions could just auto pull latest master create custom UF2 for Trinkey with this API enabled, if maintainers decide it's not worth the balancing :D

@dhalbert
Copy link
Collaborator

It's just that having this single piece of API unavailable for some reason is counter-intuitive. Maybe it could return message indicating why it's saying NotImplementedException?

That would be a helpful message. We could raise NotImplementedError with "no room in firmware" or not included in firmware, though there are sometimes multiple reasons for something being not implemented: it could be because it wouldn't fit, or because this particular board doesn't have the pins available, etc.

@domints
Copy link
Author

domints commented Nov 11, 2024

I think having message like "Excluded during build process" could still be useful in that it would indicate that it was purposefully excluded, in contrast to somebody introducing a bug or just forget to implement something. It could lead me to looking at the firmware build process instead of creating this issue in the first place - I honestly believed this to be a bug :D

@dhalbert
Copy link
Collaborator

I agree we could have a better message:
"not provided on this board"
"no room in firmware"
"not implemented on this port"
etc.
I will change the title of this issue.

@dhalbert dhalbert changed the title Adafruit Trinkey Neo throws NotImplementedError on set_usb_identification NotImplementedError could give reason why not implemented Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants