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 a generic GPIO driver for Allwinner SoCs and Orange Pi Zero/Lite/Lite2 pin map #1130

Merged
32 commits merged into from
Feb 11, 2021

Conversation

ZhangGaoxing
Copy link
Contributor

No description provided.

@ZhangGaoxing
Copy link
Contributor Author

I moved the code to Iot.Device.Gpio, but how to reference https://github.com/dotnet/iot/tree/master/src/System.Device.Gpio/Interop/Unix and System.Device.Gpio 😅
@krwq @joperezr

@joperezr
Copy link
Member

joperezr commented Jul 6, 2020

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.

@ZhangGaoxing
Copy link
Contributor Author

@krwq @joperezr

@ZhangGaoxing
Copy link
Contributor Author

Is anybody there? @krwq @joperezr

@joperezr
Copy link
Member

Sorry @ZhangGaoxing for some reason this PR fell through the cracks and we missed it. Will review now.

@joperezr
Copy link
Member

joperezr commented Aug 27, 2020

Can we change the project name? I would make this be two bindings, one would be the OrangePi, and the other one would be Sunxi one. Each one with its own namespace and samples.

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

@joperezr
Copy link
Member

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);
Copy link
Member

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?

Copy link
Contributor Author

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😅

@ZhangGaoxing
Copy link
Contributor Author

@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))
Copy link
Member

Choose a reason for hiding this comment

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

consider using TryGetValue

Copy link
Contributor Author

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))
{
}

Copy link
Member

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

Copy link
Member

@krwq krwq left a 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

Copy link
Member

@krwq krwq left a 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.

@microhobby
Copy link
Contributor

microhobby commented Jan 20, 2021

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 /dev/mem, since we already have an implementation that covers all SoC generically using the Linux Kernel interfaces, which use the hardware resources and handle exceptions in the best possible way.

@ZhangGaoxing don't get me wrong, nothing personal.

@ZhangGaoxing
Copy link
Contributor Author

@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. 😄

@microhobby
Copy link
Contributor

@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.

@ZhangGaoxing
Copy link
Contributor Author

ZhangGaoxing commented Jan 21, 2021

@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.😄

@microhobby
Copy link
Contributor

@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 😁

Base automatically changed from master to main January 29, 2021 01:47
@joperezr
Copy link
Member

@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 /dev/gpiomem for most cases, but it could also go into /dev/mem as well in some other cases.

Copy link
Member

@joperezr joperezr left a 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!

@joperezr joperezr added Auto-Merge When added to a PR, it will be automatically merged after all checks pass. and removed needs review 🔍 labels Feb 11, 2021
@ghost
Copy link

ghost commented Feb 11, 2021

Hello @joperezr!

Because this pull request has the Auto-Merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit fe22a80 into dotnet:main Feb 11, 2021
ZhangGaoxing added a commit to ZhangGaoxing/iot that referenced this pull request Mar 15, 2021
…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]>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Device.Gpio Contains types for using general-purpose I/O (GPIO) pins Auto-Merge When added to a PR, it will be automatically merged after all checks pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants