-
Notifications
You must be signed in to change notification settings - Fork 594
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 a generic GPIO driver for Allwinner SoCs and Orange Pi Zero/Lite/Lite2 pin map #1130
Conversation
I moved the code to |
Not sure what you mean on how to reference System.Device.Gpio or why you would need that, but referencing native PInvokes should be easy by just making sure that your project also compiles those interop files. They don't expose any surface area and all the purpose they have is to be able to PInvoke to native libraries that S.D.Gpio already depends on so I don't see harm on doing that. Of course, it will be important for you to think of what to do in the Windows case. |
Sorry @ZhangGaoxing for some reason this PR fell through the cracks and we missed it. Will review now. |
Actually scratch that, I just noticed that the OrangePi driver is a subclass of the Sunxi ones, so it is fine to keep as one binding but do please rename it to be OrangePi binding as opposed to Iot.Device.Gpio |
Left some initial feedback. Once that is addressed I'm happy to take another look and do let me know if you need help developing this. BTW, if we find that it is really hard to develop a Gpio driver outside of System.Device.Gpio, we can discuss about moving this OrangePi drivers into System.Device.Gpio which might make things easier. |
/// <param name="callback">Delegate that defines the structure for callbacks when a pin value changed event occurs.</param> | ||
protected override void RemoveCallbackForPinValueChangedEvent(int pinNumber, PinChangeEventHandler callback) | ||
{ | ||
_interruptDriver.OpenPin(pinNumber); |
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.
why do we need to open pin in order to remove the callback?
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.
I referred to RaspberryPi3LinuxDriver.cs
before coding😅
iot/src/System.Device.Gpio/System/Device/Gpio/Drivers/RaspberryPi3LinuxDriver.cs
Line 200 in ddeecef
_interruptDriver.OpenPin(pinNumber); |
@joperezr @krwq Does anyone come to review this PR? After this PR is merged, I can continue to add the GPIO driver of Rockchip. 😄 https://github.com/ZhangGaoxing/rockchip-gpio-driver |
*cfgPointer = cfgValue; | ||
*pulPointer = pulValue; | ||
|
||
if (_pinModes.ContainsKey(pinNumber)) |
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.
consider using TryGetValue
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.
To eliminate this warning, I need extra steps to deal with the situation where pinState is null (although it cannot be null). 😅
// VS warns me that pinState may be null
if (_pinModes.TryGetValue(pinNumber, out PinState pinState))
{
}
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.
I'm not sure what you mean but fine as is
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.
LGTM after fixing copyright headers, remainder is optional feedback
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.
2 recommendations left, let me know if you prefer to merge as is.
Just commenting that I don't agree with this. It is not the correct way to go on adding all possible GPIO implementations for each existing SoC vendor messing up with @ZhangGaoxing don't get me wrong, nothing personal. |
@microhobby Yes, what you said makes sense. But I want to ask a question directly. How do I operate a pin that is set to non-GPIO mode? How do I use what you call Linux Kernel interfaces to pull up a GPIO pin? In fact, most of the GPIO operations can be done using SysFsDriver, but this PR is just to implement some special operations of GPIO. According to your point of view, actually there is no need for WiringPi. 😄 |
@ZhangGaoxing yes, in my view WiringPi is not needed at all. We already have pull-up, pull-down configurations in GPIOLIB. Pin multiplexing must be done in Kernel space by device trees or overlays. I already mentioned this here: #1397 And by the way, if you miss something that should be done in Kernel space at GPIOLIB I'm sure your technical knowledge will be welcome in the Linux Kernel community. When this is added to the Kernel pinctrl subsystem there is a whole mechanism that deals generically, and handles exceptions to a kernel level taking into account the entire system, with all implementations of SoC vendors that kernel has support. IMHO this is the correct way to follow. |
@microhobby Actually, before you come to this conclusion, you have made the assumption that libgpiod is available on any device. ARM is different from x86, after a product (board) is launched, the Linux kernel version will probably never change again. We can't guarantee that these new technologies can be used in any ARM board. This is why I choose to inherit SysFsDriver to implement the SunxiDriver, because libgpiod is not available on the sunxi boards I tested (for now). Although sysfs is no longer recommended, the popularity of libgpiod may take much longer than we think. Maybe you use more Raspberry Pi, and the maintenance of Raspberry Pi is also more timely, but Raspberry Pi is not the best choice, because there are cheaper and better performance boards, and the number of people using these boards is far more than those using Raspberry Pi. This driver just gives them a choice. By the way, I read your blog. You are a professional, ha-ha, my work is web related, not hardware development. We look at the problem from different perspectives. You are from the perspective of the development of the industry, while I am more from the perspective of users. Several issues have mentioned this problem. I just want to solve it.😄 |
@ZhangGaoxing If you are going to use this because the vendor does not have updated software or does not provide means for you to compile updated software, especially the kernel, it is a mistake to choose this hardware from that vendor. But is not the case of Alwinner SoCs, they are very well maintained and have DTS updated by the community in mainline. And it is not difficult to compile a Kernel and upgrade to a ready to use updated distro that has packages with the latest version of libgpiod, and others by far more critical updated packages. I assure you that it is much less work and time spend than all this work that you make here, it's just a matter of configuration, everything is there. But, I really understand your point of view. Users are so used to just using the software that the manufacturer of the development board provides that they forget that first it is a development board for prototyping and second according to the software that is provided generally, most of the time, it was not meant to last a 10-year life cycle product but only as a basis, you should not use it as something reliable for your project, there are a lot of security issues and other details. My point is, we should use the interfaces here in the right way and recommend the most reliable way to use the system resources since the beginning, I have seen many "temporary solutions" made during the development go to the field with the promise "we will fix this later", and only be fixed when a issue breaks out. My opinion here is an alert, what I want and I think that all contributors of the project want, it is whoever uses the library to succeed in their projects, apply it in production in final products and earn money with it (why not 🤑). But many projects start out as something more "maker" DIY and it's develops to a final product, so the importance of starting in the "pro" way from the beginning. Another point, using the interfaces that the updated versions of the Kernel provide also helps us in maintaining the project here. Having different drivers for different platforms for different SoC vendors only adds to the complexity of project maintenance. To be clear, I really appreciate your work here, my intention is not to discourage you, just giving my humble opinion based on my experience. Obs.: and I use more boards from the NXP iMX family 😁 |
@microhobby I understand your concern, and it is very valid. One of the reasons why we are not currently opting for adding all of these other drivers into the main System.Device.Gpio library is because we currently think that the ideal driver for non-Raspberry Pi's should be libgpiod driver. That said, we shouldn't block the addition of faster (even if they are more dangerous) drivers to the bindings library, as the user has to do a conscious decision in order to use them and instantiate this driver and pass it to the controller. We can add a note on the driver's notes and mention that the driver is dealing with internal memory registers, so it is dangerous. That said, we don't currently have that note on our RaspberryPiDriver which is also using registers directly (it will only 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.
Sorry it took so long to get this in. Thank you for all the hard work @ZhangGaoxing!
Hello @joperezr! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
…Lite2 pin map (dotnet#1130) * add cs files * add csproj * resolve some feedbacks * MSBuild fail * derive from UnixDriver * reference source files * reference csproj * resolve some issues * using GpioController * _interruptController is assigned a value in the constructor * _interruptController nullable type * nullable check * SunxiDriver instead of inheriting SysFsDriver; add samples * add readme; update samples * add pin mode check; add static to _pinNumberConverter * call initialize in constructor * update README.md * fix init problem; add some validates * add Orange Pi Lite driver * clean up; fix cpux-port initial problem; remove internal exceptions * remove tiny delay * private opi zero _pinNumberConverter * add xml comment * update README.md * fix error SA1208 * fix copyright headers * cleanup * fix error SA1028 * fix CPUX port offset * count the number of pins; update README.md * fix typo * Fix Interop duplication Co-authored-by: Jose Perez Rodriguez <[email protected]>
No description provided.