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

Read only mac address rp2040 #7358

Merged
merged 3 commits into from Jan 2, 2023

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Dec 19, 2022

EDIT: Also fixes #7357.

https://forums.adafruit.com/viewtopic.php?p=952638: user found AttributeError message confusing when trying to set MAC address on Pico W. The attribute exists, despite the message, but is not settable (currently?) for CYW43.

  • Used NotImplementedError instead, with a clarifying message.
  • Share that message with another place.
  • Merge "read-only` messages to reduce number of messages.

@dhalbert
Copy link
Collaborator Author

Will revise to fix #7357.

@jepler
Copy link
Member

jepler commented Dec 19, 2022

>>> class X:
...     @property
...     def p(self): return 1
... 
>>> x = X()
>>> x.p
1
>>> x.p = 7
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: can't set attribute

Python uses AttributeError for a non-settable property but it has a clearer error ("can't set" vs "has no attribute").

Maybe we should fix this by making the failure to set the attribute have a distinct message:

diff --git a/py/runtime.c b/py/runtime.c
index 6be5c22335..89d459648a 100644
--- a/py/runtime.c
+++ b/py/runtime.c
@@ -1247,7 +1247,7 @@ void mp_store_attr(mp_obj_t base, qstr attr, mp_obj_t value) {
     mp_raise_AttributeError(MP_ERROR_TEXT("no such attribute"));
     #else
     mp_raise_msg_varg(&mp_type_AttributeError,
-        MP_ERROR_TEXT("'%s' object has no attribute '%q'"),
+        MP_ERROR_TEXT("'%s' object can't set attribute '%q'"),
         mp_obj_get_type_str(base), attr);
     #endif
 }

It'd be up to you whether to have the sites you changed here use this style of message or the style of message that you already used.

@jepler
Copy link
Member

jepler commented Dec 19, 2022

(or save even more code space by making the property either a GET or a GETSET depending on #defines, which would let this site's error checking be reused)

@jepler
Copy link
Member

jepler commented Dec 27, 2022

@dhalbert let me know when you update this PR

@dhalbert
Copy link
Collaborator Author

dhalbert commented Jan 2, 2023

@jepler on CYW43, is the fact you can't set the MAC address for station or AP mode inherent, or just not implemented yet? That affects my thinking about what error to raise.

I am changing the the AttributeError message in py/runtime.c to be "can't set attribute '%q'", which will match CPython.

@dhalbert dhalbert force-pushed the read-only-mac-address-rp2040 branch from 832879d to 03b43b7 Compare January 2, 2023 16:46
@jepler
Copy link
Member

jepler commented Jan 2, 2023

As far as I can tell the mac address is not settable in cyw43.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thank you!

@dhalbert
Copy link
Collaborator Author

dhalbert commented Jan 2, 2023

Tested on Pico W and Metro ESP32-S2.

@dhalbert dhalbert merged commit d1cd096 into adafruit:main Jan 2, 2023
@dhalbert dhalbert deleted the read-only-mac-address-rp2040 branch January 2, 2023 18:23
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.

Fix error when trying to set cpu.frequency but not supported
2 participants