Skip to content

Conversation

brentru
Copy link
Member

@brentru brentru commented May 15, 2025

This pull request introduces new component definitions for I2C output devices and a schema to validate these definitions.

New Component Definitions:

  • Added 7-Segment LED Matrix definition, including properties like I2C addresses, output type, alignment, and brightness. (components/i2c_output/7Seg/definition.json)
  • Added Quad Alphanumeric Display definition, specifying properties such as I2C addresses, output type, alignment, and brightness. (components/i2c_output/QuadAlpha/definition.json)
  • Added 16x2 Character Display definition, detailing properties like I2C addresses, output type, and display dimensions. (components/i2c_output/charDisplay16x2/definition.json)
  • Added 20x4 Character Display definition, including properties such as I2C addresses, output type, and display dimensions. (components/i2c_output/charDisplay20x4/definition.json)
  • Added SSD1306 Monochrome OLED display definition and related properties.

Resolves:
#254
#252
#251

@brentru brentru requested review from lorennorman and tyeth May 15, 2025 20:48
@brentru
Copy link
Member Author

brentru commented May 15, 2025

@lorennorman After renaming the files, the validation step seems stuck trying to query the old filename:

❌ components/i2c_output/7Seg/definition.json (no uppercase characters allowed)

@tyeth
Copy link
Member

tyeth commented May 16, 2025

Looks like the files still have uppercase characters in their names according to the changed files tab of this PR.
Also the quad should use image.png instead of quad.png
image

Copy link
Member

@tyeth tyeth left a comment

Choose a reason for hiding this comment

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

Looks great, although I'm not so sure on the i2c_output folder, maybe we add a direction(INPUT/OUTPUT/BLANK) to i2c components, or we plan for components that include multiple subcomponents.

Copy link
Contributor

@lorennorman lorennorman left a comment

Choose a reason for hiding this comment

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

this isn't ready for review, the auto-checker has not been satisfied and has valid complaints:

  • cannot have capital letters in director names: 7Seg, chartDisplay...
  • component image must be named image.png (not quad.png)

@brentru
Copy link
Member Author

brentru commented May 19, 2025

cannot have capital letters in director names: 7Seg, chartDisplay...

Hm, I'm not sure why GH isn't picking this change up. They are l-case on my system. I'm going to git mv with force to attempt to resolve this.

component image must be named image.png (not quad.png)

Renamed!

@brentru
Copy link
Member Author

brentru commented May 19, 2025

Realized I did not fully add i2c_output to the validate.yml workflow, enabled that

@brentru brentru requested a review from lorennorman May 19, 2025 16:40
@brentru
Copy link
Member Author

brentru commented May 19, 2025

@lorennorman Okay, fixed all the issues from the automated checks and added the i2c_output's schema to validate.yml to ensure the definitions are being validated against the schema.

Copy link
Contributor

@lorennorman lorennorman left a comment

Choose a reason for hiding this comment

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

💯

@brentru
Copy link
Member Author

brentru commented May 19, 2025

@lorennorman Is this safe to merge?

@lorennorman
Copy link
Contributor

@brentru no, we need to make changes on the IO side to support a whole new component type.

@brentru
Copy link
Member Author

brentru commented May 19, 2025

Ok. Hopefully this PR helps you scope out the breadth/depth of this work.

Copy link
Member

@tyeth tyeth left a comment

Choose a reason for hiding this comment

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

Couple of tweaks, but looks great, nice work adding all these!

@brentru
Copy link
Member Author

brentru commented May 21, 2025

@tyeth Updated the JSON definitions to resolve your comments. Thank you for submitting them!

@brentru brentru merged commit cad8fcd into main Jun 18, 2025
14 checks passed
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