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.

Sign up for GitHub

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

Jump to bottom

Renamed UNIXSocketType enum and added ALL_PARAMETERS interface configuration definition. #323

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Renamed UNIXSocketType enum and added ALL_PARAMETERS interface configuration definition. #323

wants to merge 13 commits into from

Conversation

adamshapiro0
Copy link
Collaborator

@adamshapiro0 adamshapiro0 commented Jun 12, 2024 •
edited
Loading

New Features

  • Added InterfaceConfigType::ALL_PARAMETERS and config structs (e.g., TCPConfig) to allow bulk configuration of a transport
  • Added GetConfigMessage(interface=) argument for ease of use, consistent with SetConfigMessage

Changes

  • Renamed SocketType to UNIXSocketType for clarity

Fixes

  • Fixed Python encoding for TransportDirection and SocketType values

@adamshapiro0 adamshapiro0 requested a review from axlan June 12, 2024 12:10
@adamshapiro0 adamshapiro0 self-assigned this Jun 12, 2024
@adamshapiro0 adamshapiro0 force-pushed the all-params branch 3 times, most recently from c077d3f to 5dbecdd Compare June 12, 2024 12:41
axlan
axlan requested changes Jun 12, 2024
View reviewed changes
Copy link
Collaborator

@axlan axlan left a comment

Choose a reason for hiding this comment

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

This is missing the Python implementation, which is probably going to be non-trivial. Similarly, the Nautilus implementation may be a little complicated since it's going to need to return multiple memory segments, which we've done for wheel speed, but I'm not sure we've done for the interface settings.

Base automatically changed from doxygen to master June 14, 2024 15:11
@adamshapiro0 adamshapiro0 force-pushed the all-params branch 2 times, most recently from 222c606 to 4e35913 Compare June 25, 2024 19:18
@adamshapiro0
Copy link
Collaborator Author

@axlan added Python implementation and resolved a few related issues. Please take another look.

@adamshapiro0 adamshapiro0 changed the base branch from master to position-error July 3, 2024 11:53
Base automatically changed from position-error to master July 3, 2024 12:13
anathan
anathan reviewed Aug 12, 2024
View reviewed changes
src/point_one/fusion_engine/messages/configuration.h
uint8_t enabled = 1;
TransportDirection direction = TransportDirection::SERVER;
uint16_t port = 0;
char remote_address[64] = {0};
Copy link
Member

Choose a reason for hiding this comment

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

its odd to call this remote address if this represents a server and a client. My personal opinion is that TCPServer and TCPClient should be treated as two different top level entities. There is no such thing as a remote address on a TCP server and if its intended to be the bind_address, it should be named as such.

anathan
anathan approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member

@anathan anathan left a comment

Choose a reason for hiding this comment

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

overall the name changes make sense.

axlan
axlan approved these changes Aug 13, 2024
View reviewed changes
Copy link
Collaborator

@axlan axlan left a comment

Choose a reason for hiding this comment

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

LGTM

@adamshapiro0 adamshapiro0 force-pushed the all-params branch 2 times, most recently from 31b0cfd to 7a89556 Compare August 19, 2024 16:06
@adamshapiro0 adamshapiro0 force-pushed the all-params branch 2 times, most recently from dce9cfe to b8321cc Compare October 29, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axlan axlan axlan approved these changes

@anathan anathan anathan approved these changes

Assignees

@adamshapiro0 adamshapiro0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamshapiro0 @anathan @axlan