-
Notifications
You must be signed in to change notification settings - Fork 174
Add support for libgpiod v2+ API #1725
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
|
@mhei |
|
CI is a fine thing 😄 I see that I changed the fallback behavior unintendedly. I'll have a look and rework. |
43196d5 to
7c635e5
Compare
|
At the moment, I did not add stuff for autotools based build. I've seen that there is some (larger?) refactoring/improvements ongoing. Should I add the required changes here already now, or should this be added later? |
7c635e5 to
cef77ee
Compare
The basic autotools build is pretty good alrready. So I I tend to think it is good to add the support now. |
cef77ee to
4722745
Compare
|
Thanks for the PR. Let me try this on my Raspberry Pi 400 over the weekend. |
|
I am using a fully up-to-date Raspberry Pi OS with my Raspberry Pi 400 and it still comes with libgpiod v1. |
|
But the package name is confusing. |
|
So I just build the latest libgpiod from its git repo. It is not compatible with git main. |
|
But this PR is good. |
|
However, this PR does not seem to work. I will check later. I am testing PR #1714 and it works. |
|
Just wondering if you can review this PR as well. Thanks. As of now, it does not seem to work with my Raspberry Pi 400. On the other hand, the current git main does not seem to be compatible with the libgpiod v2+ API. So we may need to improve this PR to get it working, |
4722745 to
f2df416
Compare
Stupid error on my side, sorry for this. This should be fixed now. |
|
Somehow it is still not working. |
|
Thanks again, I'll have a look next weekend. |
f2df416 to
ddcc52b
Compare
|
It gave me no peace before weekend. So at least reading the flash looks good to me. Then I switched over to the libgpiod-dev which comes as Debian package for Raspberry Pi OS. It could read the flash as well, compared the two files with sha256sum showed same hash. But I also cannot export the GPIO manually, I've to check later. |
Nice, that is a good progress.
It may be an existing problem. You may have to reboot the Raspberry Pi and see if the fall-back is working or not. The following PR makes the sysfs interface a bit better but not sorted out all the issue. libgpiod is the way to go. So PR #1299 was merged and it makes the situation much better. Now with libgpiod v2+ API change, I think your PR will be a good improvement. |
|
I found the reason, why the original/example configuration file does not work anymore. I'm using a Raspberry Pi 4 with latest image and the GPIO numbers changed. After a quick search I stumbled over e.g. raspberrypi/linux#5668 where a reason for renumbering is given. we see that the first gpiochip is now with offset 512. That means, that we need to add 512 to all GPIO numbers in the config file when using sysfs. When using libgpiod, then the GPIO numbers to use are just the offsets within the relevant gpiochip, so we can stay with the current numbers. Thanks so far for all your patience and testing, really appreciated. |
Good finding. Thanks for the updates. I think this is a sensible solution. |
Better to figure out from the kernel what the base number is. How would a user otherwise know which configuration to use? |
For the sysfs case, figuring out means that you have to look in the directory /sys/class/gpio to see what gpiochipXYZ devices are available. The default "port" (compiled in) is "gpiochip0". The simplest way for the user would be to just override the default by giving "gpiochip512". Then it would be possible to use the value from the port string as offset while opening the pins. But this is not 1:1 compatible with the libgpiod path: since /sys/class/gpio/gpiochip512 is registered as /dev/gpiochip0 character device, the valid configuration when using libgpiod would still be using port "gpiochip0", because libgpiod does not bother with /sys/class/gpio stuff at all. So what should be the goal here? Same config works with both ways? |
That it just works™. Assuming that AVRDUDE can write some code to figure out what the GPIO offset is with each kernel version, old or new, it would be much better to let AVRDUDE figure out how to get to the pins rather than each and every user having to know what the kernel does and which config entry to use as a consequence. It's linuxgpio, so I am assuming we don't need to worry too much about many differet platforms.
Yes, that would be great. I am not in a position to write that code, but it looks like it must be possible. Would you like to give it a go, @mhei? |
|
@stefanrueger In fact I am favoring to remove the sysfs support since it has been deprecated for quite a while and will become a maintenance nightmare for a small project like avrdude. Reference: |
|
@stefanrueger and @mhei Removing sysfs support is my recommendation. But if it is not too troublesome, then I am fine with keeping the sysfs support as well. The problem is that there are many Single Board Computers out there and I am not so sure if we can support them well, since they may use a modified kernel. As of now, we mainly support Raspberry Pi series and we rely on the users of other boards (eg: Orange Pi) to report the issue and even fix the issue. BTW, probably the following issue is also related to sysfs change. But once the user switches to libgpio, the problem got resolved. I am not so sure if the following issue is also related to sysfs. I have posted the question back to the issue reporter. |
|
@stefanrueger and @mhei Yet another option is to keep sysfs code as it is (only working with older kernel version). New kernel version users will be required to use libgpiod. I tend to think this is probably a good compromise. How do you like the idea? |
|
Now this PR works well for libgpiod v2+ API change. |
|
I will suggest we merge this PR first to address the issue of libgpiod v2+ API change. The reason is that git main no longer works (linking errors) if the user's system has the upgraded libgpiod. Then we can have another issue to address the sysfs change for new kernel version. How do you like this idea? |
Same here with a fully up-to-date Raspberry Pi OS installation with my Raspberry Pi 400. |
OK, happy to merge during the next mergefest if
@mhei please confirm once done
@mcuee Thanks for explaining; I have looked at the code and now see that sysfs is a fallback when libgpiod is not available. According to https://www.thegoodpenguin.co.uk/blog/stop-using-sys-class-gpio-its-deprecated/ it has been deprecated since 2015, but it's OK AVRDUDE still uses |
src/linuxgpio.c
Outdated
| #ifdef HAVE_LIBGPIOD_V2 | ||
|
|
||
| struct gpiod_line { | ||
| struct gpiod_chip *chip; |
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.
Please use two spaces instead of one tab
If the user has not upgraded to the new libgpiod v2 API and still use the older v1 API, git main and this PR both work fine. Please refer to earlier comment. As of now Raspberry Pi OS still ships with older version of libgpiod library with older v1 API. And I tested this PR with the stock v1 version of libgpiod inside Rasberry Pi OS and it worked well. If the user has upgraded to new libgpiod version, then git main will fail to link due to API change. This PR now works in this situation. |
libgpiod in version 2 or above introduced an API change which results in compile error with the current code. This commit adds some glue magic for the newer versions and tries to detect the used libgpiod version based on the information available in the pkg-config files. At the moment, this eliminates the possibility to statically link against this library. Signed-off-by: Michael Heimpold <[email protected]>
ddcc52b to
ea701bc
Compare
I understand the idea, but this will not work. This is a conceptual problem. As already mentioned by @mcuee, I'm unsure how the whole gpio stuff looks on other platforms (SBCs). I think we have the following options:
I would keep the current sysfs code as is. The reason is that it still gives the highest flexibility. I'd do an AVRDUDE release with support for both libgpiod v1 and v2 support ASAP. @stefanrueger: using two spaces instead of tabs now, thanks. |
|
@mhei Thanks for the update The AVRDUDE project likes to be backward compatible, and as such is unlikely to remove support for libgpiod v1 or sysfs for that matter. I understand your point of view. I think(!) I have read up sufficiently on sysfs and gpiod to hazard a guess that it ought to be possible to try a workaround once AVRDUDE realises that the current config doesn't work with the current kernel. Knowing that some kernes shifted the GPIO numbering from one version to the next, AVRDUDE could try to undo that shift by carefully looking at the config GPIO numbers and the GPIO numbers offered by the kernel. |
|
#1751 created to capture your improvement idea for sysfs fallback. |
libgpiod in version 2 or above introduced an API change which results in compile error with the current code.
This commit adds some glue magic for the newer versions and tries to detect the used libgpiod version based on the information available in the pkg-config files.
At the moment, this eliminates the possibility to statically link against this library - sorry for this, maybe it is possible to re-add this later again.
To give a big picture: I'm not a user of avrdude and I could only compile test this change so far - run-time testing and review is really appreciated. I'm libgpiod package maintainer for OpenWrt and this package/tool needs be buildable against the newer libgpiod otherwise it would block the libgpiod update or it would be required to disable this function subset. This is something I would like to prevent, so thanks for helping out here with testing and review.