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

ssd1306: Add function SetFlip and GetFlip #702

Merged
merged 2 commits into from
Aug 18, 2024

Conversation

notpop
Copy link
Contributor

@notpop notpop commented Aug 16, 2024

Implementation of the Flip Functions

Hello, I am new to contributing to OSS, so I apologize in advance if there are any mistakes.

What I did

I added a Flip functions to the ssd1306 driver.

Background

While playing with ssd1306, I wanted to flip the display, but I found that there was no method implemented for it. I had to directly execute commands within the application. Thus, I thought it would be helpful to implement this functionality in the driver itself.

Why I didn't extend SetRotation

Initially, I thought it would be best to integrate the flip functionality into SetRotation to align with the other existing implementations. However, I found that implementing 90° and 270° rotations is quite challenging. To help clarify my point, please refer to the images below:

From my current understanding, it seems that the ssd1306 hardware does not support these kinds of rotation features directly, so a more complex implementation would be required. However, I believe that even just implementing the flip functionality is useful, so I decided to start with this.

Thank you for your time and consideration.

@notpop notpop marked this pull request as ready for review August 16, 2024 07:50
Copy link
Member

@sago35 sago35 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

It is true that the SSD1306 does not seem to have 90-degree or 270-degree rotation capabilities, so the Flip API seems appropriate.
The only potential issue might be that the default value has been changed.
Originally, it was set so that IsFlip == true. I also found out that the register settings when IsFlip == true differ from the initial values of the SSD1306. However, in terms of compatibility with tinygo.org/x/drivers, I think it would be better not to change this. What do you think? @deadprogram

@deadprogram
Copy link
Member

deadprogram commented Aug 16, 2024

I think we would be better off with SetRotation like other displays instead of adding another API. The implementation could return an error if you tried to set to a currently unsupported rotation for a particular display.

@notpop notpop requested a review from sago35 August 16, 2024 15:52
@notpop
Copy link
Contributor Author

notpop commented Aug 16, 2024

Thank you for your feedback. Based on your suggestions, I have revised the SetRotation function.

@deadprogram deadprogram changed the base branch from release to dev August 18, 2024 17:40
@deadprogram
Copy link
Member

Thank you for making the changes @notpop looks good to me.

I changed the branch for this PR to dev as discussed here:
https://github.com/tinygo-org/drivers/blob/release/CONTRIBUTING.md#how-to-use-our-github-repository

@sago35 or anyone else have any comments before I squash/merge?

Copy link
Member

@sago35 sago35 left a comment

Choose a reason for hiding this comment

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

LGTM

@deadprogram deadprogram merged commit d688fa3 into tinygo-org:dev Aug 18, 2024
1 check passed
@deadprogram
Copy link
Member

Thank you @notpop and @sago35

@notpop notpop deleted the flip_ssd1306 branch August 24, 2024 20:52
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.

3 participants