Skip to content

pico-sdk: Remove unnecessary Kconfig items #84545

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

soburi
Copy link
Member

@soburi soburi commented Jan 25, 2025

  • Remove unnecessary Kconfig settings.
  • Exclude unnecessary drivers from the build target.
  • Remove the include conditions and make all include targets uniform.
  • Change claim.c to always be compiled.

These measures will delete unnecessary Kconfig items.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 25, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff

All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_rpi_pico DNM This PR should not be merged (Do Not Merge) labels Jan 25, 2025
@soburi soburi force-pushed the hal_rpi_pico_cleanup branch 5 times, most recently from bc726e5 to fe2b43f Compare February 3, 2025 03:26
@soburi soburi changed the title Hal rpi pico cleanup pico-sdk: Remove unnecessary Kconfig items Feb 3, 2025
@soburi soburi marked this pull request as ready for review February 3, 2025 03:39
@zephyrbot zephyrbot added area: PWM Pulse Width Modulation area: DMA Direct Memory Access platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) area: SPI SPI bus area: UART Universal Asynchronous Receiver-Transmitter area: ADC Analog-to-Digital Converter (ADC) area: Counter labels Feb 3, 2025
@fabiobaltieri fabiobaltieri added DNM (manifest) This PR should not be merged (controlled by action-manifest) and removed DNM This PR should not be merged (Do Not Merge) labels Feb 4, 2025
- Remove unnecessary Kconfig settings.
- Exclude unnecessary drivers from the build target.
- Remove the include conditions and make all include targets uniform.
- Change claim.c to always be compiled.

These measures will delete unnecessary Kconfig items.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Reorder items to make the file easier to read.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
@soburi soburi force-pushed the hal_rpi_pico_cleanup branch from fe2b43f to c7db7c0 Compare February 15, 2025 05:48
@zephyrbot zephyrbot requested a review from ajf58 February 15, 2025 05:49
@zephyrbot zephyrbot removed manifest manifest-hal_rpi_pico DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Feb 15, 2025
Copy link
Collaborator

@ajf58 ajf58 left a comment

Choose a reason for hiding this comment

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

I'd like to see the first commit message reworded to clarify what's going on. As it's currently written (reading from top to bottom) it reads like this.

  1. The Kconfig files were unnecessary, so then I removed them.
  2. Then I remove the include conditions and make all include targets uniform.

But this is backwards: because you've made the includes unconditional (and, in some cases also removed some source files from being compiled), the Kconfig options became unnecessary, so could be removed.

This begs the question "is it better to unconditionally have a long list of include directories, or conditionally add them as required?" I prefer the latter, shorter command line calls to the pre-processor, clearer what file it required for what subsystem/driver.

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 18, 2025
@github-actions github-actions bot closed this May 2, 2025
@soburi soburi reopened this Jun 16, 2025
Copy link

@github-actions github-actions bot removed the Stale label Jun 16, 2025
@soburi soburi marked this pull request as draft June 16, 2025 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ADC Analog-to-Digital Converter (ADC) area: Counter area: DMA Direct Memory Access area: PWM Pulse Width Modulation area: SPI SPI bus area: UART Universal Asynchronous Receiver-Transmitter platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants