-
-
Notifications
You must be signed in to change notification settings - Fork 80
Implement I2CDevice class on EV3 #379
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
Conversation
dlech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So many commits! I think half of my comments were on code that got added and then promptly removed.
Just a few high-level comments:
-
I'm not sure that
write_then_read()as a user API is particularly useful. Pretty much the only time you actually write then read is on devices that use register addressing, so this is already covered by theread()method.For the less common cases when a device doesn't use register addressing, I think allowing
Noneas the reg arg for theread()andwrite()methods would be simpler than adding a new method. -
For the case of multiple devices on a single port, why do we need the extra
wait()calls? It seems like this is something that should be baked in if it is actually required.
lib/pbio/drv/ioport/ioport.c
Outdated
| pbdrv_gpio_out_low(&pins->uart_buf); | ||
| return PBIO_SUCCESS; | ||
| } else if (mode == PBDRV_IOPORT_P5P6_MODE_I2C) { | ||
| // First reset all pins to inputs by going to GPIO mode recursively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the advantage of doing this recursively vs. just setting each gpio once here?
lib/pbio/drv/i2c/i2c_ev3.c
Outdated
| PBIO_OS_AWAIT_UNTIL(state, pbio_os_timer_is_expired(&i2c_dev->timer)); | ||
| pbio_os_timer_set(&i2c_dev->timer, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reversed. Timer is set after awaiting. If something funny is going on that makes this correct, we should have a comment explaining it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This ensures a minimum delay without slowing down code that polls
// less frequently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't explain why we wait for the timer to expire first and then set the duration of the timer after that. I would expect to set the timer first, then wait for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you would wait every time. If a user already has a wait in their loop, then we don't need to wait again here.
So we only wait if it isn't already expired. I'll rephrase.
| } | ||
|
|
||
| // Otherwise need to prefix write data with given register. | ||
| uint8_t write_data[256]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This amount of stack is fine on EV3, but not so sure about NXT. Most I2C operations are going to be 32 bytes or less so that could be a sensible number to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was here to avoid always allocating, but I suppose we could still do that instead.
Testing to allow a range of devices like any I2C devices is more natural this way.
So far we assumed that if a device made it out of the passive device manager, it must be a LUMP device. There may be other categories of active sensors that need a background process, so prepare for it now.
Reset isn't implemented here, so we don't need this argument.
Start off with a basic implementation. This is not async-compatible yet.
We used to turn power off only at the end of active sensor processes as well as at the end of a program. In general, we want to turn power off whenever something is disconnected, which is what we do here. The goal here is to be able to turn off 9V to a sensor if a user has switched it on and then unplugs the device.
This is only an indentation change with continue replaced by returning success. This thread should wait for a connection and then wait until disconnected. The outer sensor port loop will run this again instead. This allows us to handle things like port reset in the outer sensor loop. This ensures 9V power is turned off once a device is unplugged, for example.
This will automatically turn off when unplugged.
Also supports async API. This replaces the smbus version for ev3dev. Some missing methods will be reinstated in future commits.
This way we don't need to add the `max_read_size` parameter and only allocate as much as we need.
These are wrappers around write_then_read for backwards compatibility.
It appears that this was left on by accident.
A user could inadvertently make another async I2C request, but we don't allow this if another operation is ongoing. So we shouldn't override the state in this case.
Since this already deals with nxt quirks, include the required delay between operations too. This simplifies higher level code common to all I2C Sensors.
Creating duplicate async functions in Python for every method is not very nice. We can instead define callbacks to map bytes to relevant data when the operation completes.
Instead of having the driver memcpy it, let the caller handle it so it can do it after allocation. This also means we don't have to allocate bytes objects at all for most sensor methods in the nect commits.
We no longer need a double allocation just to shift the write data.
We've been gradually introducing this pattern for newer classes, so be consistent about it.
Allows sensor definitions to have an I2DDevice instance with awaitable operations with a result mapped to objects.
Given the firmware size limitations on NXT, using Python wasn't going to be scalable. With the recent changes to the I2CDevice implementation, we can use instances of it for sensor classes written in C. This reverts commit a37f46f756cd8453ff8f99193263ac68f9b5a057. This is reverted instead of dropped so we maintain the logic to import Python classes into the C module in git history for future reference.
This is commonly used for LEGO-style I2C device classes. Also simplify passing in address to I2CDevice object creator.
Split out setting defaults to a separate function so we don't have to call it recursively. Also make the comments about the quadrature mode never changing more explicit.
The original read/write were deemed generic enough. See #379
This is sufficient for most operations and more sensible for NXT. Larger amounts of data can still be written by prefxing the register in the user code.
A single retry appears sufficient so far. Fixes pybricks/support#2314
This implements the
pybricks.iodevices.I2CDeviceclass. It is compatible with the class we had in Pybricks 2.0, but adds new features. You can use use either LEGO-style (autodetected) devices to raiseERROR_NO_DEVif it isn't found, or disable auto-detection to allow custom devices.You can gracefully switch between them. If you run a new program with an auto-detectable sensor after using a custom sensor last time, you can do so without rebooting the hub. This takes a little bit of time (less than a second), but should be more stable than in 2.0, where it would take much longer and sometimes not revert modes.
A new method called
write_then_readis added to complement the existingreadandwritefor advanced users.Other sensors
Support for the NXT Ultrasonic Sensor and the Temperature Sensor has been added. The Energy Meter should follow shortly. Support for powered devices is added, which is needed for the Ultrasonic Sensor and selected other devices.
Async support
Unlike in 2.0 where everything was blocking, the new API is also async-compatible. You can make background loops that poll your devices without blocking the rest of your code. The internal implementation of the
I2CDeviceclass is such that we can easily add builtin support for more sensors without a big penalty on build size. Of course new sensors can just be implemented in Python too.Multiple devices
Using multiple sensors with different addresses on one port also works by initializing two
I2CDeviceinstances (or two regular sensor instances). It is then up to the user to poll them one at a time.Thanks
Big thanks to @ArcaneNibble for implementing a low-level I2C driver using the PRU.