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

esp32_board_spi:Missing Data Command pin support #6511

Merged
merged 1 commit into from
Jun 28, 2022
Merged

esp32_board_spi:Missing Data Command pin support #6511

merged 1 commit into from
Jun 28, 2022

Conversation

adamkaliszan
Copy link

Summary

E-ink SSD1680 and Esp bugfix

Impact

None

Testing

Lilygo t5v5 wieth 2.9 e-ink display

gustavonihei
gustavonihei requested changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@gustavonihei gustavonihei left a comment

Choose a reason for hiding this comment

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

Change request to avoid code duplication and move some fixed GPIO mapping from Kconfig to the board interface header.

@xiaoxiang781216
Copy link
Contributor

@adamkaliszan please rebase to the latest code to fix the ci build error.

@davids5 davids5 changed the title Missing Data Command pin support esp32_board_spi:Missing Data Command pin support Jun 25, 2022
@davids5
Copy link
Contributor

davids5 commented Jun 25, 2022

@adamkaliszan Please scope your (see title edit) PR messages so that the email subjects contain what part/arch/board of the system is effected.

@pkarashchenko
Copy link
Contributor

Seems that the rebase is done in a wrong way.

@acassis
Copy link
Contributor

acassis commented Jun 26, 2022

Seems that the rebase is done in a wrong way.

What issue did you see? @adamkaliszan did you follow the documentation: https://nuttx.apache.org/docs/latest/contributing/making-changes.html#submitting-your-changes-to-nuttx ?

@adamkaliszan
Copy link
Author

adamkaliszan commented Jun 27, 2022 •
edited
Loading

The commit is fine. I have decided to withdrawn possibility of configuration LCD Command/Data pin.
The reason is following: it is necessary to modify all boards.h files in esp32 directory. Possibility of configuration Command/Data pin after writing menu config is confusing, because it is still required proper modification of board.h file for every new BSP. Unfortunately there is no "master" board.h file for every eps32 board.

@gustavonihei
Copy link
Contributor

The commit is fine. I have decided to withdrawn possibility of configuration LCD Command/Data pin. The reason is following: it is necessary to modify all boards.h files in esp32 directory. Possibility of configuration Command/Data pin after writing menu config is confusing, because it is still required proper modification of board.h file for every new BSP. Unfortunately there is no "master" board.h file for every eps32 board.

The changes on this PR are specific to a single board, why would the change from this PR affect the board.h for every BSP?
Neither of the ESP32-based boards already supported in NuttX provides this e-ink display out-of-the-box, so they shouldn't be affect.

@adamkaliszan adamkaliszan force-pushed the ssd1680_esp32_support branch 2 times, most recently from eedf618 to eb71c1d Compare June 27, 2022 18:14
@adamkaliszan adamkaliszan force-pushed the ssd1680_esp32_support branch 2 times, most recently from ec17199 to f123e2c Compare June 28, 2022 03:15
@pkarashchenko pkarashchenko self-requested a review June 28, 2022 10:13
pkarashchenko
pkarashchenko approved these changes Jun 28, 2022
View reviewed changes
Copy link
Contributor

@pkarashchenko pkarashchenko left a comment

Choose a reason for hiding this comment

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

LGTM!

@gustavonihei gustavonihei merged commit 7df7989 into apache:master Jun 28, 2022
@gustavonihei
Copy link
Contributor

Thanks for the contribution, @adamkaliszan!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gustavonihei gustavonihei gustavonihei approved these changes

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

@pkarashchenko pkarashchenko pkarashchenko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@adamkaliszan @xiaoxiang781216 @davids5 @pkarashchenko @acassis @gustavonihei