Skip to content

BENB lcd color and sync#150

Merged
benblaise-intech merged 23 commits intomasterfrom
BENB-lcd-color-and-sync
Feb 5, 2025
Merged

BENB lcd color and sync#150
benblaise-intech merged 23 commits intomasterfrom
BENB-lcd-color-and-sync

Conversation

@SukuWc
Copy link
Member

@SukuWc SukuWc commented Jan 31, 2025

No description provided.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@benblaise-intech benblaise-intech changed the title Benb lcd color and sync BENB lcd color and sync Feb 3, 2025
codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@benblaise-intech benblaise-intech self-assigned this Feb 3, 2025
codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

Copy link
Member Author

@SukuWc SukuWc left a comment

Choose a reason for hiding this comment

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

Changes look comprehensive, overall quality and style matches the standards of the code base, please consider implementing the minor possible improvements mentioned in the comments!

uint8_t a;
};

#define grid_unpack_rgba(color) \
Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this not a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can this be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

grid_gui_lua_draw_demo should exercise the lua api instead of just calling draw demo. It is important because this can throw error if there were incompatible API changes!

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that this change was necessary for graphics demos but must be reverted before merging to master!

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that this change was necessary for graphics demos but must be reverted before merging to master!

GRID_LCD_CLK_COUNT,
};

struct grid_esp32_lcd_model {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can we refactor to have two instances of esp32_lcd_model instead of making every member variable an array to support multiple screens?


esp_lcd_panel_handle_t handle = lcd->panel[lcd_index][GRID_LCD_CLK_FAST];

gpio_set_level(lcd->cs_gpio_num[lcd_index], 0);
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 would be nice to create a wrapper function for activating and deactivating an lcd, instead of raw GPIO calls

codescene-delta-analysis[bot]

This comment was marked as outdated.

Copy link
Member Author

@SukuWc SukuWc left a comment

Choose a reason for hiding this comment

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

Nice job, ready to merge!

@benblaise-intech benblaise-intech marked this pull request as ready for review February 5, 2025 12:50
@benblaise-intech benblaise-intech merged commit 90f477a into master Feb 5, 2025
7 checks passed
@Greg-Orca Greg-Orca mentioned this pull request Mar 28, 2025
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants