Conversation
|
Looks like tests need to be fixed. |
|
Also seems like you grabbed some commits off a different branch when you created this one. You can fix that using interactive rebase. |
Gold856
left a comment
There was a problem hiding this comment.
Looks pretty good at first glance. The big question I have is boards that support setting LEDs via a command of some sort, but not via diozero. If we do want to support LEDs on that kind of board, we still need CustomGPIO (I think). If not, we should clean up our config to not have those command fields.
|
I am currently planning to leave custom GPIO commands unimplemented, since The commit history and unit tests will be fixed before the draft status of this PR is removed. |
d46aad5 to
2c18a10
Compare
photon-core/src/main/java/org/photonvision/common/hardware/VisionLED.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/VisionLED.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/VisionLED.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/util/math/MathUtils.java
Outdated
Show resolved
Hide resolved
|
I am revisiting custom GPIO before unmarking this as a draft. I have some new ideas on how to make that happen. |
|
Can we make a PR to https://github.com/PhotonVision/photon-image-modifier as well to remove the installation of pigpio which is no longer needed? |
This was on my plans. |
275fd35 to
1915638
Compare
|
I elected to break compatibility with the older GPIO commands due to differences in behavior for the new GPIO commands. Specifically, the presence of new GPIO command dictates if they are enabled, instead of the platform. Also, |
photon-core/src/main/java/org/photonvision/common/configuration/HardwareConfig.java
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/HardwareManager.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/VisionLED.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/VisionLED.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/util/ShellExec.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/photonvision/common/hardware/GPIO/CustomDigitalInputOutputDevice.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/GPIO/CustomDeviceFactory.java
Outdated
Show resolved
Hide resolved
|
An option for |
|
If there's a good use case for |
|
This is the IC that an LL3 uses to drive its LEDs: https://www.diodes.com/assets/Datasheets/AL8860.pdf. The data sheet recommends a PWM frequency under 500Hz, so it is probably good to make it configurable. |
|
Then yeah, you can add it. |
|
This is the IC that an LL2+ uses to control its LEDs, which doesn't appear to have any PWM input frequency recommendations: www.ti.com/lit/ds/symlink/lm3407.pdf |
359ccb6 to
c37b54a
Compare
|
The default PWM frequency is 50Hz. I confirmed that the frequencies and duty cycles were being set accurately through an oscilloscope. I also confirmed that LL3 leds act weird with frequencies significantly above 500Hz, with no apparent difference between 50Hz and 500Hz. |
|
I am considering bypassing |
|
Since |
7380661 to
8ba8450
Compare
…to avoid confusion
bd17745 to
3907a78
Compare
photon-core/src/main/java/org/photonvision/common/hardware/GPIO/CustomAdapter.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/VisionLED.java
Outdated
Show resolved
Hide resolved
photon-core/src/test/java/org/photonvision/hardware/HardwareTest.java
Outdated
Show resolved
Hide resolved
photon-core/src/main/java/org/photonvision/common/hardware/GPIO/CustomAdapter.java
Outdated
Show resolved
Hide resolved
|
Ooh, if we're gonna completely remove all the old GPIO classes and put in new ones, we may as well adjust the package name so that |
|
I think I got the package renamed? Changing the capitalization of a folder when the underlying filesystem treats folders as case-insensitive is tricky. |
Gold856
left a comment
There was a problem hiding this comment.
Looks great! Thank you so much for all the work you put into this and for seeing it through to the end! We really appreciate the long overdue cleanup.
Corresponding to PhotonVision/photonvision#2171, this removes the now-unused `pigpio` dependency from the installation scripts. closes #63
The old GPIO abstraction was replaced with [`diozero`](https://www.diozero.com), which supports most hardware running Linux due to its use of GPIO character devices provided by the Linux kernel. `diozero` also supports [alternate providers](https://www.diozero.com/concepts/providers.html#providers) if for some reason the character device API is insufficient. Certain capabilities outside of the character device API is also implemented for common hardware. Custom GPIO commands are implemented via a custom `diozero` provider. The configuration for custom GPIO will need manually updated according to the Hardware Config documentation page. The status LED class was also reworked to support additional statuses or LED indication types, although none have been added yet. This was tested on a RPi 5 with LL3 illumination LEDs and an RGB status led attached. All capabilities worked as expected. All 8 status LED colors were tested and functional via modifying the code. Basic functionality of custom GPIO was tested with dummy commands.
The old GPIO abstraction was replaced with [`diozero`](https://www.diozero.com), which supports most hardware running Linux due to its use of GPIO character devices provided by the Linux kernel. `diozero` also supports [alternate providers](https://www.diozero.com/concepts/providers.html#providers) if for some reason the character device API is insufficient. Certain capabilities outside of the character device API is also implemented for common hardware. Custom GPIO commands are implemented via a custom `diozero` provider. The configuration for custom GPIO will need manually updated according to the Hardware Config documentation page. The status LED class was also reworked to support additional statuses or LED indication types, although none have been added yet. This was tested on a RPi 5 with LL3 illumination LEDs and an RGB status led attached. All capabilities worked as expected. All 8 status LED colors were tested and functional via modifying the code. Basic functionality of custom GPIO was tested with dummy commands.
Description
The old GPIO abstraction was replaced with
diozero, which supports most hardware running Linux due to its use of GPIO character devices provided by the Linux kernel.diozeroalso supports alternate providers if for some reason the character device API is insufficient. Certain capabilities outside of the character device API is also implemented for common hardware.Custom GPIO commands are implemented via a custom
diozeroprovider. The configuration for custom GPIO will need manually updated according to the Hardware Config documentation page.The status LED class was also reworked to support additional statuses or LED indication types, although none have been added yet.
This was tested on a RPi 5 with LL3 illumination LEDs and an RGB status led attached. All capabilities worked as expected. All 8 status LED colors were tested and functional via modifying the code. Basic functionality of custom GPIO was tested with dummy commands.
closes #2062
Meta
Merge checklist: