Skip to content

Conversation

@soburi
Copy link
Member

@soburi soburi commented May 16, 2022

adc_read() that defined in adc.h of PICO-SDK conflicts with zephyr's ADC API.
Rename it to avoid compile errors.

PICO-SDK doesn't reference adc_read() from itself,
so we can change the definition only.

Signed-off-by: TOKITA Hiroshi [email protected]

@soburi
Copy link
Member Author

soburi commented Aug 22, 2022

Hi, @petejohanson, @andrei-edward-popa,
I'm sorry to bother you while you're busy, but could you please give me a review?
(I couldn't assign as reviewers, so please comment.)

@mbolivar-nordic
Copy link

@stephanosio @carlescufi @nashif @anangl I think we should merge this patch, despite the maintenance headache it implies. CI results are available in the zephyr PR linked above. Any objections?

@petejohanson
Copy link

Not to nitpick (nitpico?), But would prefixing it be more standard? pico_adc_read?

adc_read() that defined in adc.h of PICO-SDK conflicts with zephyr's ADC API.
Rename it to avoid compile errors.

PICO-SDK doesn't reference adc_read() from itself,
so we can change the definition only.

Signed-off-by: TOKITA Hiroshi <[email protected]>
@soburi
Copy link
Member Author

soburi commented Aug 26, 2022

Hi, @petejohanson

Thank you for reviewing.

Not to nitpick (nitpico?),

😆

But would prefixing it be more standard? pico_adc_read?

Change #1 is avoiding conflict using by prefixing.
I should fix same as it.

Thanks for insightful pointing out!

@str4t0m
Copy link

str4t0m commented Sep 2, 2022

May I suggest to create a list of such patches either in the README.md, the CONTRIBUTING.md, or a new file such that these patches won't be forgotten in future sdk updates.
Including the MHZ to PICO_MHZ patch of the first PR and the renaming of adc_read() in this PR for now.

@stephanosio
Copy link
Member

I think we should merge this patch, despite the maintenance headache it implies. CI results are available in the zephyr PR linked above. Any objections?

Given the status quo that Zephyr functions are not prefixed with a Zephyr-specific prefix, agreed.

@soburi
Copy link
Member Author

soburi commented Sep 3, 2022

Hi, @stephanosio @str4t0m

I think we should merge this patch, despite the maintenance headache it implies. CI results are available in the zephyr PR linked above. Any objections?

Given the status quo that Zephyr functions are not prefixed with a Zephyr-specific prefix, agreed.

Added ChangeLog.zephyr.md to record changes.

Comment on lines 5 to 8
## [cd0bef7](https://github.com/zephyrproject-rtos/hal_rpi_pico/commit/cd0bef7) - 2022-08-26
- Rename adc_read() to pico_adc_read()
## [191f5ba](https://github.com/zephyrproject-rtos/hal_rpi_pico/commit/191f5ba) - 2022-02-02
- Patch occurrences of KHZ/MHZ to PICO_KHZ/PICO_MHZ
Copy link
Member

Choose a reason for hiding this comment

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

The commit SHAs given here are from the PR branch and will no longer be valid once this branch is merged.

I would suggest adopting the approach documenting the list of patches and the files changed: https://github.com/zephyrproject-rtos/hal_st/blob/cccbc24c14decfd3f93959f7b14514536af973c7/sensor/stmemsc/README#L60-L62

If one wants to find out which commit, they can easily do so by using git blame anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, please do the suggested change @soburi

@@ -0,0 +1,12 @@
# The List of changes to fit zephyr.
Copy link
Member

Choose a reason for hiding this comment

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

Please squash the last two commits.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephanosio
Thank you for checking.
Rebasing done. Could you verify it?

Add ChangeLog.zephyr.md file to log changes in the Zephyr.

Signed-off-by: TOKITA Hiroshi <[email protected]>
@carlescufi carlescufi merged commit a094c06 into zephyrproject-rtos:zephyr Sep 6, 2022
@soburi soburi deleted the rename_adc_read branch September 6, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants