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

Add I2C support and support for INA219 device (from pi-ina219) #1070

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TiborVoelcker
Copy link

Hi, all!

I needed I2C support for Remote GPIO so I added some bits and pieces to get it working. I tried to keep it as close to the SPI implementation as possible by pretty much copying everything.

This only includes a PiGPIOHardwareI2C implementation for now. I also did not add any additional documentation (except the copied docstrings).
For now I only added the i2c_read_byte_data , i2c_read_word_data , i2c_write_byte_data and i2c_write_word_data to read/write single bytes/words. This might not be the best solution, but worked for me.

I can work on this further for a bit, if this implementation is wanted. But I wanted to check first before I put more hours in.

As I was adding I2C to communicate with a INA219, I also added this as a new I2C device. This implementation is entirely stolen from chrisb2's pi_ina219. It has MIT license, but I'm not sure how to attribute him correctly or if I should add his MIT license somewhere?

@TiborVoelcker
Copy link
Author

I can also add a example file for using the INA219, which I used to read out my sensor over Remote GPIO.

Also, to add some links: If this implementation is wanted, this could help resolve #422, #573, #802, #808 and probably some others too.

Please let me know what kind of interface is wanted for reading the I2C. Is the read/write for byte/word enough? Maybe it should be also made clear which endianness is used.
Of course, I can also add documentation at the end.

@TiborVoelcker TiborVoelcker marked this pull request as ready for review March 14, 2023 11:48
@lurch
Copy link
Contributor

lurch commented Mar 15, 2023

I've no idea if this is suitable (that's a @waveform80 decision), and I have no time to test any of it (I've never done any I2C stuff), but I'll leave some review-comments anyway in case they're useful 🙂

@@ -55,6 +57,34 @@ def spi_port_device(clock_pin, mosi_pin, miso_pin, select_pin):
raise SPIBadArgs('invalid pin selection for hardware SPI')


I2C_HARDWARE_PINS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe also include the additional I2C ports available on the Pi4? https://elinux.org/RPi_BCM2711_GPIOs
But I dunno how you'd want to deal with the way that some I2C ports are available on multiple pins 🤷‍♂️

Copy link
Author

Choose a reason for hiding this comment

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

I was only looking at https://de.pinout.xyz/pinout/i2c.
I will gladly add them, but I am a bit confused: In your source, there are SDA/SCL 1-6 defined, but 2 is missing?
Also, what are the different banks? SDA/SCL 0 is defined for every bank, do I just add all of them?
And lastly: e.g. SDA/SCL 6 is defined as the same as SCA/SCL 0. I can add this, no problem. But what is going on there?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was only looking at https://de.pinout.xyz/pinout/i2c.

See pinout-xyz/Pinout.xyz#322 😉

You probably want to have a look at https://datasheets.raspberrypi.com/bcm2711/bcm2711-peripherals.pdf for more in-depth information. But to answer your specific questions...

there are SDA/SCL 1-6 defined, but 2 is missing?

I2C-2 and I2C-7 are used by the HDMI ports, and aren't available on the GPIO pins.

Also, what are the different banks?

On the Raspberry Pi 4 only the GPIOs in bank 0 are available on the GPIO header (see the column labelled "RPi4 connection" in that table). I think some of the bank 1 GPIOs are available on the CM4, but I can't remember off the top of my head. Easiest option is to just ignore bank 1 for now.

And lastly: e.g. SDA/SCL 6 is defined as the same as SCA/SCL 0. I can add this, no problem. But what is going on there?

GPIO0 when set to ALT-mode 0 gives you access to the SDA0 signal. GPIO0 when set to ALT-mode 5 gives you access to the SDA6 signal. Similarly, GPIO22 when set to ALT-mode 5 would also give you access to the SDA6 signal.
The different ALT-modes for each GPIO pin allow you to choose which signals you want mapped to each GPIO pin. See also some of the discussion in pinout-xyz/Pinout.xyz#457

gpiozero/pins/pi.py Outdated Show resolved Hide resolved
Comment on lines +144 to +146
bus_adc=ADC_12BIT, shunt_adc=ADC_12BIT):
"""Configure and calibrate how the INA219 will take measurements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this should be part of the __init__ method, rather than being a separate configure method? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Like I said, this was completley stolen from chrisb2's pi_ina219.
I can add a call to configure to __init__, but it might be left out on purpose. If you want to use the device with the same settings as it already has (without knowing or caring what they are), you can use it without calling the configure method.
Just as a note to make sure. I will gladly add a call to configure in __init__

Copy link
Contributor

Choose a reason for hiding this comment

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

Again this is a call for @bennuttall or @waveform80 to make, but IIRC GPIO Zero tries to be much "higher level" / "easier to use" than other typically lower-level libraries.

gpiozero/pins/pi.py Outdated Show resolved Hide resolved
@TiborVoelcker
Copy link
Author

I also need help figuring out the Copyright notices and License issue. Regarding chris and possibly myself.

gpiozero/i2c_devices.py Outdated Show resolved Hide resolved
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.

None yet

2 participants