Skip to content
This repository was archived by the owner on Jun 30, 2021. It is now read-only.

Conversation

@Gadgetoid
Copy link
Contributor

This would have silently created a property on the class instance, rather than writing the register.

In a nutshell, this is why using properties in this way - while super tidy - is a bad idea. It's extremely easy to mistype a property name, and since Python will happily create this property instead of throwing an AttributeError you'll be none the wiser.

There must be a way to lint for this- but I couldn't find anything more graceful than __slots__ which is tediously verbose and may have unintended side-effects.

This would have silently created a property on the class instance, rather than writing the register.
@tannewt tannewt requested a review from siddacious June 5, 2020 17:26
@siddacious siddacious merged commit 9b3091f into adafruit:master Jun 5, 2020
@siddacious
Copy link
Contributor

@Gadgetoid I've definitely been bitten by the issue you mentioned with properties so I'm all for coming up with a more robust solution. Having a linter or other automated check catch them would be nice but I'm open to other suggestions as well. Feel free to open an issue on https://github.com/adafruit/Adafruit_CircuitPython_Register with any ideas.

I'm rather fond of the Register library but I'll be the first to admit that it's due for a re-think

@Gadgetoid
Copy link
Contributor Author

Ha! And I'm rather fond of my i2c-device library, but I'd also be first in line to criticise it.

My favourite possible solution to this problem is generative i2c bindings, using a device descriptor to auto-generate the code. As much as this idea tickles me I can immediately see all the ongoing maintenance issues with it (people changing the generated code instead of the course, and so on).

I love how clean the Register library is versus the huge glob of runtime definition I use in i2c-device, see this horror:

https://github.com/pimoroni/bme280-python/blob/a2da3f1c12b0c12bc630fc1b31021f06f3c29a1d/library/bme280/__init__.py#L123-L206

i2c-device does some chilling things under the hood to make this work and is also subject to typos, failing with cryptic errors. Masking out multiple fields in multi-byte registers is also particularly onerous. Doing this in lieu of a struct.unpack is... yeah:

https://github.com/pimoroni/bme280-python/blob/a2da3f1c12b0c12bc630fc1b31021f06f3c29a1d/library/bme280/__init__.py#L184-L205

But I'm keen to be able to express an i2c device in a declarative manner and not have to fiddle with bit shifting or conversion more than once.

With Register I guess you can overload __setattr__ to refuse to set attributes that aren't already in __dict__.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants