Skip to content

Conversation

@jerrysxie
Copy link
Contributor

Refactor names in transport to make some the concept more clear, no functional change.

@jerrysxie jerrysxie added the enhancement New feature or request label Jan 16, 2025
@jerrysxie jerrysxie self-assigned this Jan 16, 2025
Copy link
Contributor

@JamesHuard JamesHuard left a comment

Choose a reason for hiding this comment

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

Would like to see the method send() added back

@jerrysxie jerrysxie marked this pull request as ready for review January 16, 2025 17:00
@jerrysxie jerrysxie requested a review from akshat2112 January 16, 2025 17:00
@RobertZ2011
Copy link
Contributor

Overall I think we should also consider a rename of transport to something like message or messaging. I think that better describes the functionality.

@jerrysxie
Copy link
Contributor Author

Overall I think we should also consider a rename of transport to something like message or messaging. I think that better describes the functionality.

Yeah, I have seen people getting tripped up on this because they see transport as going onto the physical bus, so they think all messages are going onto external bus.

  • message::send()
  • message::register_endpoint()
  • message::MessageDelegate or message::MessageReceiver

@JamesHuard Thoughts?

@JamesHuard
Copy link
Contributor

Overall I think we should also consider a rename of transport to something like message or messaging. I think that better describes the functionality.

Yeah, I have seen people getting tripped up on this because they see transport as going onto the physical bus, so they think all messages are going onto external bus.

  • message::send()
  • message::register_endpoint()
  • message::MessageDelegate or message::MessageReceiver

@JamesHuard Thoughts?

My only concern with calling it "message" is that this might lead people to use it as the primary and only IPC method across the system. The initial intention was to simply offer a layer of agnostic routing between external and internal endpoint protocols, so that subsystems could coalesce these features into a single dispatch unit, and we could avoid duplicate code and ID tracking across various protocol-based services (like HID, FFA, etc.).

Moving towards 'message' as the primary unit of interaction across the system might become difficult to maintain (from a subsystem's perspective) if we have things that aren't protocols going over this communications layer.

Could something like "comms" be sufficient? This might be more intuitive than "transport", as it might imply a more bidirectional relationship between internal and external endpoints, which is where it sounds like the confusion is most manifested.

comms::Message, comms::register_endpoint(), etc.? My gut feeling here is that this would be less likely to encourage the sort of "general message passing" that "message" might incidentally.

@jerrysxie
Copy link
Contributor Author

Overall I think we should also consider a rename of transport to something like message or messaging. I think that better describes the functionality.

Yeah, I have seen people getting tripped up on this because they see transport as going onto the physical bus, so they think all messages are going onto external bus.

  • message::send()
  • message::register_endpoint()
  • message::MessageDelegate or message::MessageReceiver

@JamesHuard Thoughts?

My only concern with calling it "message" is that this might lead people to use it as the primary and only IPC method across the system. The initial intention was to simply offer a layer of agnostic routing between external and internal endpoint protocols, so that subsystems could coalesce these features into a single dispatch unit, and we could avoid duplicate code and ID tracking across various protocol-based services (like HID, FFA, etc.).

Moving towards 'message' as the primary unit of interaction across the system might become difficult to maintain (from a subsystem's perspective) if we have things that aren't protocols going over this communications layer.

Could something like "comms" be sufficient? This might be more intuitive than "transport", as it might imply a more bidirectional relationship between internal and external endpoints, which is where it sounds like the confusion is most manifested.

comms::Message, comms::register_endpoint(), etc.? My gut feeling here is that this would be less likely to encourage the sort of "general message passing" that "message" might incidentally.

@RobertZ2011 @JamesHuard

  • comms::send()
  • comms::register_endpoint()
  • comms::MessageDelegate or comms::InboxDelegate

Thoughts?

@JamesHuard
Copy link
Contributor

Overall I think we should also consider a rename of transport to something like message or messaging. I think that better describes the functionality.

Yeah, I have seen people getting tripped up on this because they see transport as going onto the physical bus, so they think all messages are going onto external bus.

  • message::send()
  • message::register_endpoint()
  • message::MessageDelegate or message::MessageReceiver

@JamesHuard Thoughts?

My only concern with calling it "message" is that this might lead people to use it as the primary and only IPC method across the system. The initial intention was to simply offer a layer of agnostic routing between external and internal endpoint protocols, so that subsystems could coalesce these features into a single dispatch unit, and we could avoid duplicate code and ID tracking across various protocol-based services (like HID, FFA, etc.).
Moving towards 'message' as the primary unit of interaction across the system might become difficult to maintain (from a subsystem's perspective) if we have things that aren't protocols going over this communications layer.
Could something like "comms" be sufficient? This might be more intuitive than "transport", as it might imply a more bidirectional relationship between internal and external endpoints, which is where it sounds like the confusion is most manifested.
comms::Message, comms::register_endpoint(), etc.? My gut feeling here is that this would be less likely to encourage the sort of "general message passing" that "message" might incidentally.

@RobertZ2011 @JamesHuard

  • comms::send()
  • comms::register_endpoint()
  • comms::MessageDelegate or comms::InboxDelegate

Thoughts?

I like Inbox delegate, it helps enforce the mail-service/mail-box like behavior of the comms service

@RobertZ2011
Copy link
Contributor

I agree with Jimi's concerns over people focusing too much on it as the only IPC method so I think comms works well. I also prefer InboxDelegate for a name. I think changing the method name from process to something like receive would be good as well. Is there anybody who isn't familiar with these APIs that we could loop in? It would be nice to get an opinion from someone who doesn't already have experience with the transport service.

@JamesHuard
Copy link
Contributor

I agree with Jimi's concerns over people focusing too much on it as the only IPC method so I think comms works well. I also prefer InboxDelegate for a name. I think changing the method name from process to something like receive would be good as well. Is there anybody who isn't familiar with these APIs that we could loop in? It would be nice to get an opinion from someone who doesn't already have experience with the transport service.

+1 to receive, that was actually the original name of that function

@jerrysxie
Copy link
Contributor Author

I agree with Jimi's concerns over people focusing too much on it as the only IPC method so I think comms works well. I also prefer InboxDelegate for a name. I think changing the method name from process to something like receive would be good as well. Is there anybody who isn't familiar with these APIs that we could loop in? It would be nice to get an opinion from someone who doesn't already have experience with the transport service.

@madeleyneVaca and @akshay-rust We rename some constructs in ec-services because people were tripping up by the name and the concepts. Would you guys mind taking a look at https://github.com/pop-project/embedded-services/blob/db0b0c4235721ed66a8a20c430e110119998d0f3/embedded-service/src/comms.rs and its usage in https://github.com/pop-project/embedded-services/blob/db0b0c4235721ed66a8a20c430e110119998d0f3/examples/rt685s-evk/src/bin/mock_espi_service.rs? To check if the new naming scheme makes this service more clear for you guys. Thanks!

@jerrysxie jerrysxie merged commit 742867d into OpenDevicePartnership:main Jan 22, 2025
45 checks passed
@jerrysxie
Copy link
Contributor Author

jerrysxie commented Jan 22, 2025

@JamesHuard @RobertZ2011 My bad, I clicked on auto-merge. I thought after I reset the approvals, it will not auto-merge, but I guess lesson learned. If there is extra feedback, feel free to comment on this PR. I will make a new PR to address the feedbacks.

@jerrysxie jerrysxie changed the title RFC: Refactor transport Refactor transport Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants