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.

Sign up for GitHub

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

Jump to bottom

boards: shields: Adding support for the waveshare pico ups-b shield #60384

Merged
merged 1 commit into from Nov 27, 2023
Merged

boards: shields: Adding support for the waveshare pico ups-b shield #60384

merged 1 commit into from Nov 27, 2023

Conversation

joeker64
Copy link
Contributor

Adding documentation and support for the Waveshare pico ups-b shield. I have also added a alternate nodelabel for the Raspberry Pi Pico i2c 'pico_i2c'

@zephyrbot zephyrbot added platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) area: Shields Shields (add-on boards) labels Jul 14, 2023
soburi
soburi requested changes Jul 14, 2023
View reviewed changes
boards/shields/waveshare_ups/Kconfig.shield Outdated Show resolved Hide resolved
boards/shields/waveshare_ups/waveshare_pico_ups_b.overlay Outdated
*/

&pinctrl{
i2c1_default: i2c1_default{
Copy link
Member

Choose a reason for hiding this comment

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

Please add space before {.

Copy link
Member

Choose a reason for hiding this comment

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

Not yet fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Offtopic: Please don't push the Resolve conversation button.
Reviewers usually forget "What I said in yesterday".
It is important sign to remind that pointed out by themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay :))

boards/arm/rpi_pico/rpi_pico-common.dtsi Outdated Show resolved Hide resolved
@joeker64 joeker64 force-pushed the add_pico_ups_shield branch 3 times, most recently from 882f8b3 to b45f159 Compare July 14, 2023 18:38
@joeker64 joeker64 requested a review from soburi July 15, 2023 10:24
soburi
soburi requested changes Jul 16, 2023
View reviewed changes
boards/shields/waveshare_ups/waveshare_pico_ups_b.overlay Outdated
*/

&pinctrl{
i2c1_default: i2c1_default{
Copy link
Member

Choose a reason for hiding this comment

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

Not yet fixed.

boards/shields/waveshare_ups/waveshare_pico_ups_b.overlay Outdated
*/

&pinctrl{
i2c1_default: i2c1_default{
Copy link
Member

Choose a reason for hiding this comment

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

Offtopic: Please don't push the Resolve conversation button.
Reviewers usually forget "What I said in yesterday".
It is important sign to remind that pointed out by themselves.

boards/shields/waveshare_ups/waveshare_pico_ups_b.overlay Outdated Show resolved Hide resolved
boards/shields/waveshare_ups/waveshare_pico_ups_b.overlay Outdated Show resolved Hide resolved
boards/shields/waveshare_ups/waveshare_pico_ups_b.overlay Outdated Show resolved Hide resolved
soburi
soburi previously approved these changes Jul 24, 2023
View reviewed changes
erwango
erwango requested changes Jul 28, 2023
View reviewed changes
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, but this doesn't fit to a shield description.
A correct shield description should be compatible with several boards through the use of a common connector.
I don't see a connector in use here, so this can only be compatible with a unique board and hence doesn't match shield definiton.

See https://docs.zephyrproject.org/latest/hardware/porting/shields.html#board-compatibility

boards/shields/waveshare_ups/doc/index.rst Outdated
Comment on lines 3 to 4
Waveshare Pico UPS-B shield
#################################
Copy link
Member

Choose a reason for hiding this comment

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

Please fix under line lenght

boards/shields/waveshare_ups/doc/index.rst Show resolved Hide resolved
boards/shields/waveshare_ups/waveshare_pico_ups_b.overlay Outdated
};
};

pico_i2c1: &i2c1 {};
Copy link
Member

Choose a reason for hiding this comment

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

This is a board setting. This definition may likely not fit on all boards.

@joeker64
Copy link
Contributor Author

joeker64 commented Aug 1, 2023

Thanks for this contribution, but this doesn't fit to a shield description. A correct shield description should be compatible with several boards through the use of a common connector. I don't see a connector in use here, so this can only be compatible with a unique board and hence doesn't match shield definiton.

See https://docs.zephyrproject.org/latest/hardware/porting/shields.html#board-compatibility

The common connector would be I2C? The pico can support having multiple I2C devices with I2C0 & I2C1. The shield is setup to use the I2C1 pins. I was going to add support for I2C1 in the default pico_dts but found this commit so I included it in the shield overlay:

9eb7ee6

@github-actions
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 Oct 17, 2023
soburi
soburi previously approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

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

Please fix pointing out by @erwango.
No additional comments from me.

@github-actions github-actions bot removed the Stale label Oct 18, 2023
@joeker64 joeker64 requested a review from erwango November 1, 2023 14:28
@joeker64
Copy link
Contributor Author

I added in back support for I2C1 but set it too disabled as default. This was so I didn't have to write i2c1 device logic in the board overlay in the shield directory.

@joeker64
Copy link
Contributor Author

Is there anything I need to do for this to be merged?

@soburi
Copy link
Member

soburi commented Nov 21, 2023

Is there anything I need to do for this to be merged?

Please fix pointing out by @erwango.

#60384 (comment)
#60384 (comment)

soburi
soburi requested changes Nov 22, 2023
View reviewed changes
Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

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

Section header underline must be same as title length.

@soburi
Copy link
Member

soburi commented Nov 22, 2023

@joeker64

Commit suggestion is not allowed by CI check.
Please rebase and squash these.
And one point, my first correction is wrong.
Please correct it to the correct number of "#".

@joeker64
boards: shields: Adding support for the waveshare pico ups-b shield
5e8ba6a 
Adding support for the Waveshare pico ups-b shield with documentation

Signed-off-by: Joseph Yates <joeyatessecond@gmail.com>
@soburi
Copy link
Member

soburi commented Nov 26, 2023

@erwango

Could you take a look, again?

@fabiobaltieri fabiobaltieri merged commit 5ae648d into zephyrproject-rtos:main Nov 27, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soburi soburi soburi approved these changes

@erwango erwango erwango approved these changes

@nashif nashif Awaiting requested review from nashif

@yonsch yonsch Awaiting requested review from yonsch yonsch is a code owner

@avisconti avisconti Awaiting requested review from avisconti

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@kartben kartben Awaiting requested review from kartben

Assignees

@erwango erwango

Labels
area: Shields Shields (add-on boards) platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@joeker64 @soburi @erwango @fabiobaltieri @zephyrbot