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.

Sign up for GitHub

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

Jump to bottom

Experimental new API endpoints #3

Merged
merged 8 commits into from
Sep 9, 2019
Merged

Experimental new API endpoints #3

merged 8 commits into from
Sep 9, 2019

Conversation

Gadgetoid
Copy link
Member

This changeset introduces a new set and get method for i2cdevice. The purpose of these methods is to provide a less-bonkers approach to writing and reading values than the current (MicroPython incompatible) system of overriding getattribute and redirecting attribute lookups to our internal register dictionary.

Get

The get method accepts a register name, reads the register and returns a namedtuple of all the available fields. These changes turn accessing multiple registers from:

with device.REGISTER_NAME as REGISTER_NAME:
    status = REGISTER_NAME.get_status()
    interrupt = REGISTER_NAME.get_interrupt()

To:

my_register= device.get('REGISTER_NAME')
my_register.status
my_register.interrupt

Note: It is important to understand that my_register is, at this point, a stateless namedtuple that's disconnected from the hardware. If it's a non-volatile register (IE: it never changes) then you can keep this object for the lifetime of your program. If you want to read it again (in the case of a volatile register) then you must call device.get('REGISTER_NAME') again.

Set

Set cleans up the current conflation of read and write whereupon using this pattern:

with device.REGISTER_NAME as REGISTER_NAME:
    REGISTER_NAME.set_status(status)
    REGISTER_NAME.set_interrupt(interrupt)
    REGISTER_NAME.write()

Could result in errors where the user (me, usually) forgets to call write() and their expected register changes are thus never written. set is explicit about the fact it's for writing to a register, and thus does not allow for this mistake, the above code would become:

device.set('REGISTER_NAME',
                status=status,
                interrupt=interrupt)

This still allows for multi-register writes, but it is disambiguated from the get method and ensures your changes are always written to device. Under the hood this function reads the current register state, locks it, updates all the fields you have supplied, writes the changed register value(s) back to the device, and unlocks the register.

This change should come with a deadline for deprication of the existing, confusing and fragile API methods- removing the pattern device.REGISTER_NAME.get_field_name() in favour of device.get('REGISTER_NAME').field_name. This is a subtle aesthetic change on the face of it, but under the hood the latter method does not rely on any fragile and overzealous getattribute trickery.

@rabid-inventor and @Septolum - since you've both worked with i2cdevice I'd appreciate your feedback here. All of our libraries can, and should be migrated over to this new style pretty easily and this will - hopefully (@Septolum correct me if I'm wrong) facilitate MicroPython compatibility without having to use the ugly (and non read/write combining) device.set_field('REGISTER_NAME', 'FIELD_NAME', value) and `device.get_field('REGISTER_NAME', 'FIELD_NAME') approach.

@coveralls
Copy link

coveralls commented Aug 27, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 23

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 98.565%
Files with Coverage Reduction New Missed Lines %
i2cdevice/init.py 3 98.37%
Totals Coverage Status
Change from base Build 13: 0.2%
Covered Lines: 206
Relevant Lines: 209
💛 - Coveralls

@Gadgetoid
Copy link
Member Author

A possible flaw in this approach may be the instability of namedtuple in CircuitPython. See: https://circuitpython.readthedocs.io/en/3.x/docs/library/collections.html

There's nothing particularly special about namedtuple as a return type, it's just a fancy class that implements a property for each register field. However, reimplimenting it to avoid potential issues with CircuitPython's namedtyple might result in us bumping into the same problems that made the getattribute overloading approach untenable.

@Gadgetoid
Copy link
Member Author

Gadgetoid commented Sep 2, 2019 •
edited
Loading

Libraries using i2cdevice -

@Gadgetoid Gadgetoid mentioned this pull request Sep 2, 2019
Merged
This was referenced Sep 6, 2019
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Sep 6, 2019
Merged
Merged
Merged
@Gadgetoid Gadgetoid marked this pull request as ready for review September 9, 2019 10:10
@Gadgetoid Gadgetoid merged commit 53a3aab into master Sep 9, 2019
@Gadgetoid Gadgetoid mentioned this pull request Sep 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Gadgetoid @coveralls