Skip to content

Increase max_size in pwm.proto#188

Merged
brentru merged 2 commits into
api-v2from
increase-pwm-name-sz
May 15, 2026
Merged

Increase max_size in pwm.proto#188
brentru merged 2 commits into
api-v2from
increase-pwm-name-sz

Conversation

@brentru
Copy link
Copy Markdown
Member

@brentru brentru commented May 5, 2026

Expander pins take the form EXP_{I2c_Address}_Pin#. The max_size field for a PWM pin is 6 characters, too small for the component API to support expanders.

This pull request aims to match the size for the pin field used by pwm.proto with digitalio.options and analogio.options.

@brentru brentru requested review from lorennorman and tyeth May 5, 2026 19:51
Copy link
Copy Markdown
Member

@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.

makes sense to make them all the same across protos, but i gotta ask why 64? isn't that much larger than we'll ever need, or should i have a more open mind about that?

@brentru
Copy link
Copy Markdown
Member Author

brentru commented May 5, 2026

@lorennorman 64 is far larger than we'd ever need, yes.

Looking at the expander format, the worst-case is: EXP_0xFF_15 = 11 chars. The worst case for simple pins is 3 ('D21`).

I think a max_size of 16 could be fine, fits a2^n, and could be universally applied across these APIs.

Interested in @tyeth's thoughts, too, though!

@tyeth
Copy link
Copy Markdown
Member

tyeth commented May 6, 2026

16 sounds sensible. I wondered in future (or now) if we need the i2c bus number included in the pin name (which would fit in 16), as we don't know from this format which bus it will be on.
Presumably we keep a list of initted expanders on device and can therefor easily distinguish by i2c address only, and as our first v2 plan is a single mux for i2c I wonder if the expanders are similarly limited to single use (by intent not by design).

SPI expanders are also a thing, so we might need an ID instead of including all expander info in pin name (SPI has chip select + d/c + optional reset iirc), or some way of looking them up.

Copy link
Copy Markdown
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.

See comment in PR about i2c bus number, and SPI expanders

@brentru
Copy link
Copy Markdown
Member Author

brentru commented May 8, 2026

@tyeth You mean moving from
Expander on bus N: EXP_{ADDR}_{BUS#}_{PIN#}
to:
Expander on bus N: EXP_{ADDR}_{BUS#}_{PIN#}

I'm open to it. In both scenarios' worst-case, we don't hit the 16char limit proposed by this PR.

@tyeth
Copy link
Copy Markdown
Member

tyeth commented May 8, 2026

@tyeth You mean moving from
Expander on bus N: EXP_{ADDR}_{BUS#}_{PIN#}
to:
Expander on bus N: EXP_{ADDR}_{BUS#}_{PIN#}

I'm open to it. In both scenarios' worst-case, we don't hit the 16char limit proposed by this PR.

Assuming that first example [the FROM] was meant to not include bus number, only the new/proposed variant includes bus, then yeah!

@brentru
Copy link
Copy Markdown
Member Author

brentru commented May 8, 2026

Assuming that first example [the FROM] was meant to not include bus number, only the new/proposed variant includes bus, then yeah!

yep my mistake!

Copy link
Copy Markdown
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.

16 not 64, but looks good and was group reviewed.

@brentru
Copy link
Copy Markdown
Member Author

brentru commented May 15, 2026

@tyeth thanks for the catch, set to 16 across the PR

@brentru brentru merged commit 632faac into api-v2 May 15, 2026
1 check passed
@brentru brentru deleted the increase-pwm-name-sz branch May 15, 2026 20:24
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