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

arm/stm32f401rc-rs485: Add support to GPIO Subsystem #15861

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rcsim
Copy link
Contributor

@rcsim rcsim commented Feb 19, 2025

Summary

This PR adds support to GPIO Subsystem for STM32F401RC RS485 board.

Impact

Users will be able to use GPIO subsystem, including adding GPIO as Output, Input and Input with Interruption.

Testing

  • Select NSH or USBNSH Config.
  • Use Menuconfig to enable CONFIG_DEV_GPIO and CONFIG_EXAMPLES_GPIO
  • Build and load the firmware in the board
  • Access NuttShell and use gpio command:
nsh> gpio -o 0 /dev/gpio1
Driver: /dev/gpio1
  Output pin:    Value=1
  Writing:       Value=0
  Verify:        Value=0
nsh> gpio -o 1 /dev/gpio1
Driver: /dev/gpio1
  Output pin:    Value=0
  Writing:       Value=1
  Verify:        Value=1
nsh> gpio /dev/gpio0
Driver: /dev/gpio0
  Input pin:     Value=0
nsh> gpio -w 1 /dev/gpio2
Driver: /dev/gpio2
  Interrupt pin: Value=0
  Verify:        Value=1

@github-actions github-actions bot added Board: arm Size: M The size of the change in this PR is medium labels Feb 19, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 19, 2025

[Experimental Bot, please feedback here]

The provided information is a good start but is missing crucial details required by the NuttX PR template. Here's a breakdown of what's missing and how to improve it:

Missing/Incomplete Information:

  • Summary: While it mentions adding GPIO support, it lacks specifics. Which GPIO pins? Were any drivers changed/added? How is the GPIO configuration handled (e.g., device tree, board configuration)? What was the motivation for adding this support (was it requested, needed for a specific application, etc.)?
  • Impact: The impact section is too general. Be specific:
    • Impact on user: How will users use this? Are there any configuration changes required? Any new APIs? Give examples.
    • Impact on build: Does this change Kconfig options? Are new files added to the build system?
    • Impact on hardware: Specifically mention the STM32F401RC. Are there any pin conflicts? Does it affect any other hardware on the board?
    • Documentation: If documentation is needed, either provide it in the PR or create a separate issue to track documentation updates. Explicitly state if no documentation updates are needed.
    • Security: While it might seem irrelevant for GPIO, consider if there are any potential security implications. For example, if a GPIO is incorrectly configured, could it expose a vulnerability? If not, explicitly state "NO".
    • Compatibility: Does this impact any existing drivers or applications? If not, explicitly state "NO".
  • Testing:
    • Build Host(s): Provide details of your development environment. Example: Linux, x86_64, GCC 12.2
    • Target(s): Be explicit. Example: STM32F401RC, sim:nsh or STM32F401RC, board:stm32f401rc-rs485_nsh (use the correct config name).
    • Testing logs before change: This is crucial. Show that the GPIO functionality didn't work before your changes. An error message, or simply stating "GPIO functionality not available" is sufficient.
    • Testing logs after change: While the provided logs demonstrate basic functionality, they are incomplete. Test different GPIO pins, input with interrupts, and error handling. Ensure the logs are complete and easy to understand.

Example of an Improved PR Description:

## Summary

This PR adds GPIO support for the STM32F401RC RS485 board, specifically enabling GPIO pins PA0-PA7.  This was needed to support [application/reason - e.g., a new sensor driver or a user request].  The implementation adds a new GPIO driver instance in the board configuration file (`boards/arm/stm32/stm32f401rc-rs485/src/stm32_bringup.c`).

## Impact

* **Impact on user:** YES. Users can now access and control GPIO pins PA0-PA7 through the `/dev/gpioN` device nodes (where N is the GPIO number). Example usage is provided in the Testing section.  Users will need to enable `CONFIG_DEV_GPIO` and `CONFIG_EXAMPLES_GPIO` in their configuration to use the GPIO driver and example application.
* **Impact on build:** YES. The build system now includes the new GPIO driver instance in the `stm32f401rc-rs485` board configuration.
* **Impact on hardware:** YES. This change specifically affects the STM32F401RC RS485 board by enabling GPIO functionality on pins PA0-PA7.  No pin conflicts are anticipated.
* **Impact on documentation:** YES.  Documentation updates have been provided in this PR to describe the new GPIO configuration and usage. (Or: NO.  The changes are self-explanatory and don't require documentation updates.)
* **Impact on security:** NO.
* **Impact on compatibility:** NO.
* **Anything else to consider:** None.

## Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux, x86_64, GCC 12.2
* Target(s): STM32F401RC, board:stm32f401rc-rs485_nsh

Testing logs before change:

nsh> gpio /dev/gpio0
gpio: /dev/gpio0: No such file or directory


Testing logs after change:

nsh> gpio -o 0 /dev/gpio0
Driver: /dev/gpio0
Output pin: Value=1
Writing: Value=0
Verify: Value=0
... (Test other GPIO pins, input functionality, and interrupt functionality)
nsh> gpio -i /dev/gpio2 (interrupt test)
... (Show interrupt triggering) ...




By providing more specific details, you'll make it much easier for reviewers to understand and approve your PR. Remember to replace the example information with the actual details of your changes.

@lupyuen
Copy link
Member

lupyuen commented Feb 19, 2025

Please remember to fill in the Commit Description. (Just copy from PR Summary) Thanks :-)

@LuchianMihai
Copy link
Contributor

Hi!
Regarding the use of

#if BOARD_NGPIOINT > 0
#endif

It's a personal preference, but,
I suggest the to use something like

#ifdef BOARD_NGPIOINT
#endif

If one or more gpio is needed, use

#define BOARD_NGPIOINT number

else do not define BOARD_NGPIOINT at all.

Current implementation assumes that BOARD_NGPIOINT should always be defined and numeric value,
Which arguably may introduce undefined behavior.

But, it's just my suggestion.

This commit adds support to GPIO Subsystem for STM32F401RC RS485 board.

Signed-off-by: Rodrigo Sim <[email protected]>
@rcsim rcsim force-pushed the stm32f401rc_rs485_gpio branch from 6dcfae8 to 7c83e56 Compare February 20, 2025 00:09
@rcsim
Copy link
Contributor Author

rcsim commented Feb 20, 2025

Please remember to fill in the Commit Description. (Just copy from PR Summary) Thanks :-)

Hi @lupyuen , sure, I've added the commit description. Thanks

@rcsim
Copy link
Contributor Author

rcsim commented Feb 20, 2025

Hi! Regarding the use of

#if BOARD_NGPIOINT > 0
#endif

It's a personal preference, but, I suggest the to use something like

#ifdef BOARD_NGPIOINT
#endif

If one or more gpio is needed, use

#define BOARD_NGPIOINT number

else do not define BOARD_NGPIOINT at all.

Current implementation assumes that BOARD_NGPIOINT should always be defined and numeric value, Which arguably may introduce undefined behavior.

But, it's just my suggestion.

Thanks for your suggestion @LuchianMihai , but if BOARD_NGPIOINT is defined as 0 (maybe by mistake), we will also have some problems. So I will keep this way. But once again, thanks for the suggestion.

rcsim@rcsim-p52:~/nuttxspace/nuttx$ git grep "BOARD_NGPIOINT 0"
boards/arm64/bcm2711/raspberrypi-4b/include/board.h:#define BOARD_NGPIOINT 0
rcsim@rcsim-p52:~/nuttxspace/nuttx$ git grep "BOARD_NGPIOINT  0"
boards/arm/at32/at32f437-mini/src/at32f437-mini.h:#define BOARD_NGPIOINT  0
boards/arm/imxrt/imxrt1050-evk/src/imxrt1050-evk.h:#define BOARD_NGPIOINT  0 /* Amount of GPIO Input w/ Interruption pins */
boards/arm/imxrt/imxrt1060-evk/src/imxrt1060-evk.h:#define BOARD_NGPIOINT  0 /* Amount of GPIO Input w/ Interruption pins */
boards/arm/imxrt/imxrt1064-evk/src/imxrt1064-evk.h:#define BOARD_NGPIOINT  0 /* Amount of GPIO Input w/ Interruption pins */
boards/arm/imxrt/imxrt1170-evk/src/imxrt1170-evk.h:#define BOARD_NGPIOINT  0  /* Amount of GPIO Input w/ Interruption pins */

@LuchianMihai
Copy link
Contributor

Sure, won't argue, but #define BOARD_NGPIOINT 0 and no define at all is the same thing. Preprocessor assigns 0 to the defines not given.

@hartmannathan hartmannathan self-requested a review February 20, 2025 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: arm Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants