Skip to content

Conversation

@ThomasDebrunner
Copy link
Contributor

This allows TCP connections to be made by hostname, not just IP - e.g. tcp://localhost:1234, or tcp://my-device.local:1234

Tested on linux and macos

Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Nice! I fixed the style, and the other CI issues seem unrelated to this PR.

@JonasVautherin JonasVautherin requested a review from julianoes May 26, 2023 09:09
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Any chance you can add the same for Windows while you're at it?

remote_addr.sin_addr.s_addr = inet_addr(_remote_ip.c_str());

struct hostent* hp;
hp = gethostbyname(_remote_ip.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
hp = gethostbyname(_remote_ip.c_str());
struct hostent* hp; = gethostbyname(_remote_ip.c_str());

As I read it, this doesn't need cleaning up, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No appranently this points to static memory

@ThomasDebrunner
Copy link
Contributor Author

I believe this should work for windows as well - does the windows CI test open a TCP connection? Otherwise I'd need to to test a real windows machine.

@julianoes
Copy link
Collaborator

CI likely doesn't test it. I'll try to test on Windows some time but you might beat me to it.

@julianoes
Copy link
Collaborator

@ThomasDebrunner sorry for the delay. Trying to build and run this on Windows caused some work 😄, see #2069. So, anyway, let me just merge it.

@julianoes julianoes merged commit ce6b718 into mavlink:main Jun 14, 2023
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