Skip to content
This repository has been archived by the owner on Sep 3, 2024. It is now read-only.

Bring the esp changes upto the latest transport (WIP) #127

Merged
merged 12 commits into from
Mar 5, 2024
Merged

Conversation

suhasHere
Copy link
Collaborator

This is rework of PR #63 . This tries to bring the esp additions to match the latest UDP transport.

This PR seems to not break non esp code as confirmed by running echo server and client.

Look forward for some early feedback and would be nice to get this moving along in coming weeks,

@suhasHere
Copy link
Collaborator Author

@TimEvens Love to get feedback on this PR. I will be testing this on esp platform in the meanwhile.

@suhasHere suhasHere requested a review from fluffy March 3, 2024 20:50
Copy link
Contributor

@TimEvens TimEvens left a comment

Choose a reason for hiding this comment

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

LGTM. The UDP changes will trigger a merge conflict with some changes that are coming, so it would be good to merge this sooner than later. Are you planning to add/update more before merge?

@@ -88,7 +88,7 @@ main()
{
char* envVar;

TransportRemote server = TransportRemote{ "127.0.0.1", 1234, TransportProtocol::QUIC };
TransportRemote server = TransportRemote{ "127.0.0.1", 1234, TransportProtocol::UDP };
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably make this a config... although I don't think it's urgent because I believe most for testing will modify this code anyway.

@@ -98,7 +98,7 @@ int main() {
cantina::LoggerPointer logger = std::make_shared<cantina::Logger>("ECHO");
Delegate d(logger);
TransportRemote serverIp =
TransportRemote{"127.0.0.1", 1234, TransportProtocol::QUIC};
TransportRemote{"127.0.0.1", 1234, TransportProtocol::UDP};
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as client comment.

@suhasHere
Copy link
Collaborator Author

LGTM. The UDP changes will trigger a merge conflict with some changes that are coming, so it would be good to merge this sooner than later. Are you planning to add/update more before merge?

@TimEvens yes let's merge it .. I will bring in more changes in small quantum's that needs some more work.

@suhasHere suhasHere merged commit c66c398 into main Mar 5, 2024
2 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants