xorbit / LiFePO4wered-Pi

Access library, command line tool and daemon for the LiFePO4wered/Pi module
GNU General Public License v2.0
132 stars 31 forks source link

RFE: More meaningful output when not cli not run with correct permissions #25

Open zbeekman opened 5 years ago

zbeekman commented 5 years ago

Hi,

First of all, thanks for making the amazing hardware and the great UI in software for it! I just got two LiFePO4wered/PI+ modules (18650 and 14500) for powering a pi3 b+ and a zero. These are FAR superior to anything else I have tried.

I was just reading through the product brief to configure the UPS (LiFePO4wered/PI+) for my DIY openVPN server at my parents house (it's easier to play tech support when I can actually access their hardware...) and encountered a brief moment of confusion:

I was reading through the manual and tried to execute the example command:

lifepo4wered­-cli get vbat

and a value of -1 was returned. Eventually I realized that I was being foolish, and this is an error code that was caused by lack of root privileges. I have 3 suggestions to improve the UX and clarity of using the CLI tool:

  1. Have the CLI tools print a diagnostic message when it is run without root privs and should have been run as root.
  2. Document that the CLI tool should be run as root in the product brief. A change as simple as adding a leading ampersand before the example lifepo4wered-cli commands to indicate that they are being run as the root user would be helpful.
  3. Document the error return code.

Before I realized my mistake, since the LiFePO4wered module is connected to wall power, I had assumed that this -1 code returned by get vbat was an indication of a defective battery.

Anyway, many thanks once again!

xorbit commented 5 years ago

Hi Izaak,

Thanks for the kind words and the feedback!

I can't really put in the manual that the CLI tool should be run with root privileges because that depends on the distro and how it's configured. By default, on Raspbian the tool works without privileges AKAIK. What distro are you using?

I do agree that I should print a more useful error message though. The problem is that this software has existed for a while since it also works with my previous iterations so I have to keep backward compatibility in mind. I don't want to break someone's script if they were just expecting a numeric value. But I can probably print the error to STDERR instead.

I definitely agree the documentation on this should be improved, so that's what I did first. There is now a table in the CLI tool docs that explains the error values: https://lifepo4wered.com/files/LiFePO4wered-Pi+-Product-Brief.pdf

zbeekman commented 5 years ago

Hi Patrick,

I can't really put in the manual that the CLI tool should be run with root privileges because that depends on the distro and how it's configured. By default, on Raspbian the tool works without privileges AKAIK. What distro are you using?

I'm using Raspbian. I compiled as my user, and AFAICT, followed the directions for setting up the software and installing the LiFePO4wered module exactly as specified. I think I had to run ./install... with sudo to put the executables into /usr/local. When I run the script without root, I get -1 vbat, with sudo I get a meaningful number. Maybe permissions have gotten tighter? I wouldn't be surprised if you needed root privs because you're interacting with hardware and controlling the entire machine state and behavior with the CLI script. Any way, I'll try to investigate more when I find some time.

Thanks, Zaak

On Mon, Nov 26, 2018 at 1:27 PM Patrick Van Oosterwijck < notifications@github.com> wrote:

Hi Izaak,

Thanks for the kind words and the feedback!

I can't really put in the manual that the CLI tool should be run with root privileges because that depends on the distro and how it's configured. By default, on Raspbian the tool works without privileges AKAIK. What distro are you using?

I do agree that I should print a more useful error message though. The problem is that this software has existed for a while since it also works with my previous iterations so I have to keep backward compatibility in mind. I don't want to break someone's script if they were just expecting a numeric value. But I can probably print the error to STDERR instead.

I definitely agree the documentation on this should be improved, so that's what I did first. There is now a table in the CLI tool docs that explains the error values: https://lifepo4wered.com/files/LiFePO4wered-Pi+-Product-Brief.pdf

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/xorbit/LiFePO4wered-Pi/issues/25#issuecomment-441745660, or mute the thread https://github.com/notifications/unsubscribe-auth/AAREPK5CMsJKh3Jj2Y4k-SYta8UakJxWks5uzDKigaJpZM4Yx3wd .

xorbit commented 5 years ago

It's very possible they tightened up security since I last did a fresh install. I should do that and see what happens by default when I find the time...

xorbit commented 5 years ago

I noticed you said you compiled "as my user". Does this mean you made a user different than the default pi user? It's possible pi is set up as having access to I2C by default, but your own user wouldn't be unless you added it to the i2c group.

zbeekman commented 5 years ago

@xorbit Thanks for noticing this and responding! Yes, for security reasons I typically harden my Pi's if I'm using them "in production" (personal use only... but still). This includes things like deactivating or removing the default Pi account, setting up fail2ban, ufw, etc. and creating a new user for myself with sudo privs. I'm pretty sure I didn't add myself to any additional groups besides sudoers, so this is very likely the explanation of the issue.

You're welcome to close this as PEBCAK, or keep it open to track adding a few words about this, etc. to the documentation.

mproctor13 commented 5 years ago

I am here because I thought I had a bad board or software did not work. I just installed fresh raspbian, then followed readme and can confirm that users other then pi or sudo fail.

I also confirmed that there is a i2c group and if you add other users to that group they then have permissions to use lifepo4wered-cli.

It seems a little user hostile to not include some language in the Manual/README to the effect that you need permission to access the hardware(i2c) in order to use utility. Either by adding those permissions, by using sudo, or by using the "pi" user.

I understand not wanting to break scripts, but you are generating -1 as output because you are casting false to an int and the displaying that. Then you returning successful exit code. Seems like you should notice when you fail open_i2c_bus and return non-zero exit code. Then people writing scripts based on lifepo4wered-cli would not be dependent on output to check for failure.

xorbit commented 5 years ago

I don't intend to be user hostile. :) So I've added more documentation about permissions to both the manual and the README.

Things also get a little complicated because of legacy. The CLI is just a thin wrapper around the lifepo4wered-data.c, which is the meat of the program that drives the CLI, daemon, shared object and language bindings. Initially there were no valid negative values but now there are. So I have to do a more involved rewrite some time without causing too much breakage. If I had written lifepo4wered-data.c differently from the start, things would be easier.

hj6769 commented 2 years ago

I had the same problem with a version 6 board, it was the power supply. My board with version 4 works with the same power supply, with firmware version 6 the same power supply does not work. I am now using a 5.9 volt power supply and it works fine. 3 days thought I was too stupid.

dbainbridge commented 2 years ago

I'm currently getting this with Pi 3B+. It's powering the Pi fine but can't access the device itself. Only other item connected is a USB camera:

root@allsky:/home/pi# lifepo4wered-cli get vbat
-1
root@allsky:/home/pi# ls -l /dev/i2c-1
crw-rw---- 1 root i2c 89, 1 Nov  4 23:01 /dev/i2c-1

I had also tried with pi user with same result.

Also ran i2cdetect -y 1 and does scan bus but shows -- for all entries.

xorbit commented 2 years ago

@dbainbridge does it make any difference if you use sudo with either the lifepo4wered-cli command or i2cdetect?

dbainbridge commented 2 years ago

@xorbit I tried sudo with both and was same result.

xorbit commented 2 years ago

@dbainbridge Very odd. Is this with Raspbian (or whatever they call it these days)? And nothing else on the GPIOs? Which Pi hardware?

dbainbridge commented 2 years ago

Yes, the May version of Raspbian on Pi 3B+. This is older hardware revision of the LiFePO4wered-Pi from couple years ago but I'm pretty sure I had this working on exact same Pi 3B+ back then (I had it off for a year). Nothing else is on the headers except the LiFePO4wered+. I might go out to try physically resetting it. I assume I have the headers on correctly since the Pi itself is working correctly with 12V powering the LiFePO4wered+.

dbainbridge commented 2 years ago

Think I am on to something:

lsmod | grep i2c
i2c_bcm2835            16384  1
i2c_dev                20480  2

and

sudo rmmod i2c_bcm2835
rmmod: ERROR: Module i2c_bcm2835 is in use
dbainbridge commented 2 years ago

I reflashed the sd card with new raspberry lite OS and then immediately installed the LiFePO4wered-Pi only to have the same -1 returned (tried with and without sudo and after reboot). Beginning to wonder if there is hardware issue. I have 2nd LiFePO4wered-Pi on order so should be able to test if it is Pi that is having issue or something else.

xorbit commented 2 years ago

Hmm yes, might be good to test with another unit. The odd thing is that you say it powers the Pi fine. Does it switch from pulsating/breathing green LED to steady? If so, the daemon can actually communicate with the device.