Skip to content

Conversation

@verios-google
Copy link
Contributor

Pretty self-explanatory. A controller can speak to more than one switch, best to know which switch the request is for.

@verios-google
Copy link
Contributor Author

@chrispsommers and @jonathan-dilorenzo, any other open concerns for the PR, otherwise can I get approval and merger

Copy link
Contributor

@jonathan-dilorenzo jonathan-dilorenzo left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: verios-google <[email protected]>
@jonathan-dilorenzo
Copy link
Contributor

So, if we think there's anything to discuss here, @antoninbas and @chrispsommers, then we could chat in meeting on Friday. O.w. I'd be comfortable with this getting merged as it looks now (or with whatever additional comments you have)?

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

LGTM

@chrispsommers
Copy link
Collaborator

So, if we think there's anything to discuss here, @antoninbas and @chrispsommers, then we could chat in meeting on Friday. O.w. I'd be comfortable with this getting merged as it looks now (or with whatever additional comments you have)?

I'm OK with as-is.

@verios-google
Copy link
Contributor Author

@antoninbas any other concerns or can we merge this

Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making the suggested change

I'd recommend making it explicit that 0 is not a valid device_id in a follow-up change. When I looked at the spec the other day, I couldn't find any mention of this, even though we do have a general statement that the zero value for scalar fields is usually not valid.

@verios-google
Copy link
Contributor Author

Sounds good, I'll take a stab at it when I get the time. @chrispsommers can you merge this PR

@chrispsommers
Copy link
Collaborator

Sounds good, I'll take a stab at it when I get the time. @chrispsommers can you merge this PR

If you're going to add a sentence about device_id 0 isn't valid, how about do that before we merge? It sounds trivial and will avoid yet another PR. We can merge in the meeting today, one way or the other.

@chrispsommers chrispsommers merged commit 8152793 into p4lang:main May 9, 2025
8 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.

4 participants