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

shields: display: Add Waveshare Pico OLED 1.3 #62708

Closed

Conversation

soburi
Copy link
Member

@soburi soburi commented Sep 15, 2023

Added Waveshare 1.3inch OLED module for RaspberryPi Pico.

@soburi soburi marked this pull request as ready for review September 15, 2023 13:15
@zephyrbot zephyrbot added the area: Shields Shields (add-on boards) label Sep 15, 2023
};
};

&i2c1 {
Copy link
Member

Choose a reason for hiding this comment

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

No connector provided, but I guess in specific case of Raspberry Pico (oine single board reference), it is fine. @jfischer-no thoughts ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense.
For consistency reasons, I added aliases as pico_spi0, pico_spi1, pico_i2c0, and pico_i2c1.

Copy link
Collaborator

@jfischer-no jfischer-no left a comment

Choose a reason for hiding this comment

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

I think we should better group shields by the controller/device it configures. Can it be part of the ssd1306 shield since it already supports multiple controllers from that family?

Add aliases for spi0 as pico_spi0 and also as each for
spi1, i2c0, and i2c1.
spi0 alias is already exists as pico_spi, it rename.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Renamed pico_spi to pico_spi0.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Added Waveshare 1.3inch OLED module for RaspberryPi Pico.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
@soburi
Copy link
Member Author

soburi commented Oct 9, 2023

@jfischer-no

I think we should better group shields by the controller/device it configures. Can it be part of the ssd1306 shield since it already supports multiple controllers from that family?

There is rpi_pico specific problem.

The i2c/spi pins are not fixed.
It requires to include boards/*.overlay file for applying the configuration that suits the shield.

If it puts this shield configuration into boards/shields/ssd1306,
the overlay file will also apply to another configuration.
Of course, It causes a contradiction with another shield.

I think it is good to create the shield configuration separately.

@jfischer-no
Copy link
Collaborator

If it puts this shield configuration into boards/shields/ssd1306,
the overlay file will also apply to another configuration.
Of course, It causes a contradiction with another shield.

How?
Just copy overlay files to boards/shields/ssd1306, adapt boards/shields/ssd1306/Kconfig.shield and it should work fine.

@soburi
Copy link
Member Author

soburi commented Oct 25, 2023

@jfischer-no

If it puts this shield configuration into boards/shields/ssd1306,
the overlay file will also apply to another configuration.
Of course, It causes a contradiction with another shield.

How? Just copy overlay files to boards/shields/ssd1306, adapt boards/shields/ssd1306/Kconfig.shield and it should work fine.

I want to use this shield by only adding the -DSHIELD=... option with west build.

For that, rpi_pico needs to configure i2c-pins with pinctrl.
The i2c-pins are not defined in board configurations because rpi_pico's i2c-pins can take several settings.
So, This PR also added boards/shields/waveshare_pico_oled_1_3/boards/rpi_pico.overlay for configure i2c.

I worry that if this PR's changes move into boards/shields/ssd1306/, the rpi_pico.overlay will do something bad in the configuration for other ssd1306....overlays.
(rpi_pico.overlay is specific for waveshare_pico_oled_1_3)
The overlay defined in ssd1306 is for Arduino, so I thought it would be better not to mix them, so I created it separately.

@jfischer-no
Copy link
Collaborator

I worry that if this PR's changes move into boards/shields/ssd1306/, the rpi_pico.overlay will do something bad in the configuration for other ssd1306....overlays.
(rpi_pico.overlay is specific for waveshare_pico_oled_1_3)
The overlay defined in ssd1306 is for Arduino, so I thought it would be better not to mix them, so I created it separately.

shield/foobar/boards/rpi_pico.overlay applies only to rpi_pico board, "something bad" is not to be expected. We should not have multiple shield directories with identical Kconfig.shield and similar descriptions about display controller used.

@soburi
Copy link
Member Author

soburi commented Oct 26, 2023

@jfischer-no

I worry that if this PR's changes move into boards/shields/ssd1306/, the rpi_pico.overlay will do something bad in the configuration for other ssd1306....overlays.
(rpi_pico.overlay is specific for waveshare_pico_oled_1_3)
The overlay defined in ssd1306 is for Arduino, so I thought it would be better not to mix them, so I created it separately.

shield/foobar/boards/rpi_pico.overlay applies only to rpi_pico board, "something bad" is not to be expected. We should not have multiple shield directories with identical Kconfig.shield and similar descriptions about display controller used.

I agree that your suggestion works correctly for this shield.
What I'm concerned about case that another ssd1306 shield for rpi_pico is newly added that has a different pin assignment.

For example, in the case the new shield is some_other_1306_shield_for_rpi_pico,
The directory will become as follows.

boards/shields/ssd1306/some_other_1306_shield_for_rpi_pico.overlay
boards/shields/ssd1306/waveshare_pico_oled_1_3.overlay
boards/shields/ssd1306/boards/rpi_pico.overlay

In this case, it uses the same rpi_pico.overlay by both shields.
So, I don't think this strategy will work because you will

Nothing like this is currently on the market, but I think it is a possibility that should be considered.

@jfischer-no
Copy link
Collaborator

@jfischer-no

I worry that if this PR's changes move into boards/shields/ssd1306/, the rpi_pico.overlay will do something bad in the configuration for other ssd1306....overlays.
(rpi_pico.overlay is specific for waveshare_pico_oled_1_3)
The overlay defined in ssd1306 is for Arduino, so I thought it would be better not to mix them, so I created it separately.

shield/foobar/boards/rpi_pico.overlay applies only to rpi_pico board, "something bad" is not to be expected. We should not have multiple shield directories with identical Kconfig.shield and similar descriptions about display controller used.

I agree that your suggestion works correctly for this shield. What I'm concerned about case that another ssd1306 shield for rpi_pico is newly added that has a different pin assignment.

For example, in the case the new shield is some_other_1306_shield_for_rpi_pico, The directory will become as follows.

boards/shields/ssd1306/some_other_1306_shield_for_rpi_pico.overlay
boards/shields/ssd1306/waveshare_pico_oled_1_3.overlay
boards/shields/ssd1306/boards/rpi_pico.overlay

In this case, it uses the same rpi_pico.overlay by both shields. So, I don't think this strategy will work because you will

Nothing like this is currently on the market, but I think it is a possibility that should be considered.

Because you configure 4c382cf#diff-868357f0097b87fbdf4a9cada9cbd5b33dfe302b591a24be6197c29625a640f2 for this shield but not at the board level. And whatever you do, it may conflict with any other shield using the same i2c interface.

@soburi
Copy link
Member Author

soburi commented Oct 26, 2023

@jfischer-no

I worry that if this PR's changes move into boards/shields/ssd1306/, the rpi_pico.overlay will do something bad in the configuration for other ssd1306....overlays.
(rpi_pico.overlay is specific for waveshare_pico_oled_1_3)
The overlay defined in ssd1306 is for Arduino, so I thought it would be better not to mix them, so I created it separately.

shield/foobar/boards/rpi_pico.overlay applies only to rpi_pico board, "something bad" is not to be expected. We should not have multiple shield directories with identical Kconfig.shield and similar descriptions about display controller used.

I agree that your suggestion works correctly for this shield. What I'm concerned about case that another ssd1306 shield for rpi_pico is newly added that has a different pin assignment.
For example, in the case the new shield is some_other_1306_shield_for_rpi_pico, The directory will become as follows.

boards/shields/ssd1306/some_other_1306_shield_for_rpi_pico.overlay
boards/shields/ssd1306/waveshare_pico_oled_1_3.overlay
boards/shields/ssd1306/boards/rpi_pico.overlay

In this case, it uses the same rpi_pico.overlay by both shields. So, I don't think this strategy will work because you will
Nothing like this is currently on the market, but I think it is a possibility that should be considered.

Because you configure 4c382cf#diff-868357f0097b87fbdf4a9cada9cbd5b33dfe302b591a24be6197c29625a640f2 for this shield but not at the board level. And whatever you do, it may conflict with any other shield using the same i2c interface.

Yes, it is. rpi_pico has no standard or typical configuration for pins. So, it should not define the default by system. In my understanding, Zephyr defines the default with only the minimum required. This is following the habit. So, I put the settings into the shield.

It's understandable that there is a possibility of collision, but isn't it too unfriendly to the user to have a mismatch happen in the simple case of using a single shield?
I think there are benefits to integrating from the perspective of eliminating duplication, but I feel it has a significant disadvantage in usability.
Also, since ssd1306 is a consistent group as a shield for Arduino, it feels strange to include something for rpi_pico here.

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 Dec 26, 2023
@github-actions github-actions bot closed this Jan 9, 2024
@kartben kartben removed the Stale label Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Display area: Shields Shields (add-on boards) platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants