You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/09/30 12:04:05 UTC

[GitHub] [incubator-nuttx] jarivanewijk opened a new pull request #4635: Userled driver: Implement getall by checking actual pin state

jarivanewijk opened a new pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635


   ## Summary
   The include/nuttx/leds/userled.h interface already defines the ULEDIOC_GETALL command for retrieving the current state of the LEDs. The current implementation directly returns the state as it was previously set.
   
   However, this does not necessarily reflect the actual state of the LEDs. Hardware may be broken or there might be a wrong configuration in software. We would like to change this command so it reads the actual pin state of the pins that control the LEDs, which is a more accurate representation of the LED state. This would require an additional board_userled_getall() function to be defined.
   
   This is still open for discussion. We want a way to check the actual LED state to verify that the LED on our board is correctly controlled and that the LED is mounted without short circuits. This needs to be done from user board logic or a user application. The existing ULEDIOC_GETALL command seems like a good fit, but only provides the state to which the LED should have been set, not its actual state. If the original functionality needs to be preserved, maybe the proposed functionality can be implemented as a separate command. We would just like to hear the opinion of the community. We are also open to other suggestions to implement this kind of functionality.
   
   FYI @cisvanmierlo
   
   ## Impact
   Boards and applications that previously used the generic userled driver implementation would need to implement a board_userled_getall() function. This then needs to read the logic level of the pins that control the LEDs. Exact implementation might vary between MCUs and boards.
   
   ## Testing
   I will test on S32K144EVB as soon as possible. I just wanted to start the discussion already.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] davids5 merged pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] acassis commented on pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635#issuecomment-932228429


   I agree with this modification, but why don't call it just "userled_ledget" instead of "userled_ledgetall"? Notice that userled_ledset is able to set all LEDs, but we don't call it userled_ledsetall. Just to keep things uniform/equivalent.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] jarivanewijk edited a comment on pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
jarivanewijk edited a comment on pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635#issuecomment-933414007


   Thanks for your feedback, @acassis. I used "userled_ledgetall" because the IOCTL command is named ULEDIOC_GETALL, so I was trying to keep it consistent.
   
   There are also IOCTL commands for setting a single LED (ULEDIOC_SETLED) and for setting all LEDs (ULEDIOC_SETALL). If in the future maybe an additional ULEDIOC_GETLED command (to get the state of a single LED) is to be added, then the naming will get confusing if I named it "userled_ledget" instead of "userled_ledgetall". 
   
   Naming is unfortunately not really consistent between userled_lower.c and userled_upper.c, which makes this a bit of a mess. I just felt that naming it "userled_ledgetall" would avoid most of this confusion and keep it somewhat consistent with the existing IOCTL commands and function names (userled_led and userled_ledset).
   
   I think it would be more clear if we renamed all userled_ functions; "userled_supported" can remain as it is, but "userled_led" should become "userled_setled". "userled_ledset" should become "userled_setall". Then the new one can become "userled_getall"?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] acassis commented on pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635#issuecomment-933416653


   > Thanks for your feedback, @acassis. I used "userled_ledgetall" because the IOCTL command is named ULEDIOC_GETALL, so I was trying to keep it consistent.
   > 
   > There are also IOCTL commands for setting a single LED (ULEDIOC_SETLED) and for setting all LEDs (ULEDIOC_SETALL). If in the future maybe an additional ULEDIOC_GETLED command (to get the state of a single LED) is to be added, then the naming will get confusing if I named it "userled_ledget" instead of "userled_ledgetall".
   > 
   > Naming is unfortunately not really consistent between userled_lower.c and userled_upper.c, which makes this a bit of a mess. I just felt that naming it "userled_ledgetall" would avoid most of this confusion and keep it somewhat consistent with the existing IOCTL commands and function names (userled_led and userled_ledset).
   > 
   > I think it would be more clear if we renamed all userled_ functions; "userled_supported" can remain as it is, but "userled_led" should become "userled_setled". "userled_ledset" should become "userled_setall". Then the new one can become "userled_getall"?
   
   @jarivanewijk I agree! You can, please do that in a new PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] jarivanewijk commented on pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635#issuecomment-945845156


   I rebased my PR again after renaming some of the functions in #4642. Took a bit longer to get back to this than anticipated. ;)
   
   I also added a configuration option CONFIG_USERLED_LOWER_READSTATE for the changes in this PR. I figured some users may want to keep the previous functionality or don't care about the proposed feature to check the LED pin state. Otherwise this would also require ALL boards with LEDs to be updated with a function definition to read the state of the appropriate pins. This may not even be easily possible under all architectures, because the LED pins are usually configured as a GPIO output, but it depends on the architecture how and if the output pinstate can be read.
   
   This should be ready for review now. Impact should be minimal, as the old functionality is preserved by default. I have tested the new functionality on a custom board with S32K144 MCU.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] jarivanewijk edited a comment on pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
jarivanewijk edited a comment on pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635#issuecomment-933414007


   Thanks for your feedback, @acassis. I used "userled_ledgetall" because the IOCTL command is named ULEDIOC_GETALL, so I was trying to keep it consistent.
   
   There are also IOCTL commands for setting a single LED (ULEDIOC_SETLED) and for setting all LEDs (ULEDIOC_SETALL). If in the future maybe a hypothetical ULEDIOC_GETLED command (to get the state of a single LED) is to be added, then the naming will get confusing if I named it "userled_ledget" instead of "userled_ledgetall". 
   
   Naming is unfortunately not really consistent between userled_lower.c and userled_upper.c, which makes this a bit of a mess. I just felt that naming it "userled_ledgetall" would avoid most of this confusion and keep it somewhat consistent with the existing IOCTL commands and function names (userled_led and userled_ledset).
   
   I think it would be more clear if we renamed all userled_ functions; "userled_supported" can remain as it is, but "userled_led" should become "userled_setled". "userled_ledset" should become "userled_setall". Then the new one can become "userled_getall"?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] jarivanewijk commented on pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635#issuecomment-933414007


   Thanks for your feedback, @acassis. I used "userled_ledgetall" because the IOCTL command is named ULEDIOC_GETALL, so I was trying to keep it consistent.
   
   There are also IOCTL commands for setting a single LED (ULEDIOC_SETLED) and for setting all LEDs (ULEDIOC_SETALL). If in the future maybe a ULEDIOC_GETLED command (to get the state of a single LED) is to be added, then the naming will get confusing if I named it "userled_ledget" instead of "userled_ledgetall". 
   
   Naming is unfortunately not really consistent between userled_lower.c and userled_upper.c, which makes this a bit of a mess. I just felt that naming it "userled_ledgetall" would avoid most of this confusion and keep it somewhat consistent with the existing IOCTL commands and function names (userled_led and userled_ledset).
   
   I think it would be more clear if we renamed all userled_ functions; "userled_supported" can remain as it is, but "userled_led" should become "userled_setled". "userled_ledset" should become "userled_setall". Then the new one can become "userled_getall"?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] jarivanewijk commented on pull request #4635: Userled driver: Implement getall by checking actual pin state

Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on pull request #4635:
URL: https://github.com/apache/incubator-nuttx/pull/4635#issuecomment-933481500


   I just opened another PR to address these concerns. After that PR gets merged I will continue work on this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org