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 support for PiJuice HAT #730

Merged
merged 8 commits into from
Oct 7, 2020
Merged

Conversation

aander07
Copy link
Contributor

This PR adds support for the PiJuice HAT (https://uk.pi-supply.com/products/pijuice-standard)

Included with this patch is more robust autoconf support for the 3.x and 4.x releases of the i2c-tools library, with the 4.x release adding the user space include file name i2c/smbus.h to resolve a user/kernel include file naming conflict. This will make it easier for future I2C drivers to handle both build environments.

I also fixed a compile warning in the ASEM driver where it needed sys/ioctl.h for a constant, but I did not update that driver to use the new I2C definitions since I do not have the hardware to test that it would not break anything.

$ upsc hat@localhost
battery.capacity: 1.820
battery.charge: 93.7
battery.charge.low: 25
battery.charger.status: resting
battery.current: 0.010
battery.packs: 1
battery.packs.bad: 0
battery.temperature: 48
battery.type: LiPO
battery.voltage: 4.113
battery.voltage.nominal: 4.180
device.mfr: PiJuice
device.type: HAT
driver.name: pijuice
driver.parameter.pollinterval: 2
driver.parameter.port: /dev/i2c-1
driver.parameter.synchronous: no
driver.version: 2.7.4-731-gc0a0e56
driver.version.internal: 0.9
input.current: 1.727
input.voltage: 5.014
ups.date: 2019-09-12
ups.firmware: 1.3
ups.mfr: PiJuice
ups.status: HB OL
ups.time: 23:53:56.92
ups.type: HAT

Configuration:

[hat]
driver = pijuice
port = /dev/i2c-1
desc = "PiJuice HAT"

Update the driver Makefile.am to add pijuice driver to the LINUX_I2C_DRIVERLIST
Check for both the i2c-tools 3.x and 4.x include files (i2c-dev.h/smbus.h) and set flags accordingly.

Link with the i2c library file from i2c-tools 4.x if it is available.
Update asem.c to complie cleanly by adding an include for <sys/ioctl.h>
Clean up the upsdebugX() labels
Pull the conditional I2C support code closer to the includes block
Don't report battery temp as UPS temp
Warn if we adjust shutdown_delay
Accommodate one more variation of SMBUS/I2C support  where both the headers and the libraries exist and are usable.
Check for I2C_FUNC_I2C to avoid errors caused by conflicts between linux/i2c-dev.h and linux/i2c.h
@clepple
Copy link
Member

clepple commented Sep 14, 2019

@aander07 thanks for submitting this - looks good at first glance! I would like to dive into the code a little more, but just wanted to let you know that someone has taken a look.

One thing about the HB status - the asem driver is one of only two drivers to use that flag, so it may cause confusion for users. It is not exactly the opposite of LB, since it does not trigger any particular events like a low-battery shutdown. If you want to keep that code in, it might make sense to add a variable like battery.charge.high (which I admit we don't have in the variable name document) so that users can set the threshold. Otherwise, it is probably easiest for a client to read battery.charge and compare.

@aquette / @zykh @jimklimov: other thoughts? I know we had proposed the nutdrv_* prefix for new drivers, but as you may have noticed, if the driver-specific part of the name is longer than, say, qx, it often gets cut off in ps listings. This driver also seems related to the asem.c code, which we didn't push to rename before we merged it. We also haven't documented this suggestion, especially in skel.c. Drivers get installed to their own directories, so I am inclined not to insist on a rename.

@zykh
Copy link
Contributor

zykh commented Sep 16, 2019

@clepple, I've yet to look at this... but, speaking of the driver name, in my opinion we should stand by our previous stance (not that I'm going to push for a name change, if there's no consensus over that, though).
As for ps, after all, most users only have one NUT driver running at a time, so, um...
Perhaps, we should add an issue to remember to document that (one thing I'm really good at, keep procrastinating)... ?

@aander07
Copy link
Contributor Author

Removing the HB flag is trivial, and since it is not clearly defined what the use case is for that flag, I'm happy to remove it until it is fully hashed out.

Let me know which direction that you want to go re: naming, and I'll take care of both of these items at the same time.

@jimklimov
Copy link
Member

Looks OK to me on the structural side :)

Given that we have now two consumers of I2C, would it make sense to refactor the fallback routine definitions in drivers/pijuice.c into a separate file that can be used by both? Something like common/nut-common-i2c.{c,h}? Then for the consuming code it would be a simple header and link addition, with few if any ifdef's.

Also, with nested #if* macros, I think it would be more readable to indent them by a space or two, and/or comment which #if clause is being #endif'ed.

@aquette
Copy link
Member

aquette commented Feb 9, 2020

hi @aander07
thanks for your contribution. Please just add a manpage and I'll merge

@curlyel
Copy link

curlyel commented Jun 2, 2020

Hi @aander07
...

Please just add a manpage and I'll merge

If this is the only missing thing, could you try adding it? Would be great having support for the PiJuice merged ;-)

@jimklimov
Copy link
Member

If only the presence of manpage is the show stopper, I posted aander07#1 to get one (ripped off asem.txt) and also to clean up some whitespaces in the new source.

@jimklimov jimklimov mentioned this pull request Oct 7, 2020
@jimklimov jimklimov merged commit a76ea06 into networkupstools:master Oct 7, 2020
This was referenced Nov 5, 2020
@patricknooijen
Copy link

How can i install this patch?

@jimklimov
Copy link
Member

jimklimov commented Feb 16, 2021 via email

@patricknooijen
Copy link

pi@retropie:/lib/nut $ sudo upsdrvctl start
Network UPS Tools - UPS driver controller 2.7.4
Can't start /lib/nut/pijuice: No such file or directory

Did i miss something? Compiled from master branch.

@clepple
Copy link
Member

clepple commented Feb 19, 2021

@patricknooijen be sure that you have all of the necessary i2c development headers installed.

Here is an example of a Buildbot system that doesn't have everything:

checking linux/i2c-dev.h usability... yes
checking linux/i2c-dev.h presence... yes
checking for linux/i2c-dev.h... yes
checking i2c/smbus.h usability... no
checking i2c/smbus.h presence... no
checking for i2c/smbus.h... no
checking whether i2c_smbus_access is declared... no
checking whether i2c_smbus_read_byte_data is declared... no
checking whether i2c_smbus_write_byte_data is declared... no
checking whether i2c_smbus_read_word_data is declared... no
checking whether i2c_smbus_write_word_data is declared... no
checking whether i2c_smbus_read_block_data is declared... no
checking for library containing i2c_smbus_read_byte... no
checking whether to build i2c based drivers... no 

From a quick look at this PR, you will probably need the last line to end in "yes". I don't have first-hand experience with recent i2c support, so please check the first comment in this PR for more information on what to search for.

If you are following one of the wiki pages for building, it is possible that the instructions do not include the dependencies for all of the drivers. Let us know what works, and someone can update them.

@fredforsyth
Copy link

I had a go at compiling from source, I have to say it was remarkably easy! Thank you for that.

After installing the header files with sudo apt install libi2c-dev it compiled the pijuice driver, and after adding the nut account to the i2c group with sudo usermod -a -G i2c nut it is working nicely. Very cool! Those were the only extra steps I needed to get this working that wasn't documented somewhere.

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.

8 participants