Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor get_port to use FNV-1a hash for consistency across platforms #634

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Nov 16, 2024

resolve commaai/openpilot#34031 :

the issue is caused by a port mismatch between the Mac and the device:

Attempting to connect to zmq endpoint: tcp://172.16.0.20:60942
Expected port on the device: 63557

It seems the static int get_port(std::string endpoint) function returns different port values on macOS and Linux, likely due to platform differences in how std::hash behaves.

This pr fixes the issue where the get_port function returns different port values on macOS and Linux due to platform-specific behavior of std::hash. The mismatch in port values causes connection failures in ZMQ. The solution replacesstd::hashwith a custom FNV-1a hash function to ensure consistent port values across platforms, and reducing the risk of collisions.

@deanlee deanlee changed the title Fix ZMQ issue caused by mismatched port values between macOS and Linux Refactor get_port to use FNV-1a hash for consistency across platforms Nov 16, 2024
@pbassut
Copy link

pbassut commented Nov 16, 2024

Confirmed this works!
Thank you @deanlee for such quick turn around on this one

@adeebshihadeh adeebshihadeh merged commit e621ce0 into commaai:master Nov 16, 2024
8 checks passed
@deanlee deanlee deleted the zmq_fix_get_port branch November 17, 2024 04:47
@sshane
Copy link
Contributor

sshane commented Nov 21, 2024

@deanlee this breaks PlotJuggler streaming, can you make a PR to fix?

@adeebshihadeh
Copy link
Contributor

Just need to rebuild PJ?

@devtekve
Copy link

Thanks @deanlee, this makes it easier to derive the same port from other languages!

@pbassut
Copy link

pbassut commented Nov 23, 2024

yea, confirmed PJ is broken. I wasn't even able to build/run PJ before so unsure if it was caused by this PR.

@devtekve
Copy link

devtekve commented Nov 24, 2024

I can't stream either. Last commit from OP stream worked I checked was f50ee8a67a283980ca758adca1a4f7ac39206437

On the commit prior to this MR, streaming was working 3e17f86

@devtekve
Copy link

@pbassut
Copy link

pbassut commented Dec 2, 2024

Can we confirm that PJ only needs a rebuild for this to be fixed?

@deanlee
Copy link
Contributor Author

deanlee commented Dec 4, 2024

Can we confirm that PJ only needs a rebuild for this to be fixed?

y. make sure to update to the latest master branch of common/PlotJugglere before rebuilding.

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.

Live stream using ZMQ doesn't work on Mac
5 participants