Skip to content

Conversation

@acmorrow
Copy link
Member

@acmorrow acmorrow commented Apr 9, 2025

Claude got this one started, I tidied it up a bit. Overall, I'd like to find a better way to do this that integrates with the new Instance mechanism, but getting something working is currently a higher priority than elegance/consistency. I'm happy to file a follow-up ticket later to do this more gracefully if we can accept this as an interim solution.

@acmorrow acmorrow requested a review from lia-viam April 9, 2025 01:30
@acmorrow acmorrow requested a review from a team as a code owner April 9, 2025 01:30
@acmorrow acmorrow requested review from stuqdog and removed request for a team April 9, 2025 01:30
Copy link
Collaborator

@lia-viam lia-viam left a comment

Choose a reason for hiding this comment

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

fine with merging this as is, but i wonder if going forward this may be past a certain threshold of ifdef proliferation where we want to split sources into signal_manager_win32.cpp and signal_manager_posix.cpp

@acmorrow
Copy link
Member Author

acmorrow commented Apr 9, 2025

fine with merging this as is, but i wonder if going forward this may be past a certain threshold of ifdef proliferation where we want to split sources into signal_manager_win32.cpp and signal_manager_posix.cpp

I totally agree. I'm going to file an issue to revisit the signal handler infrastructure, and note both this idea and the idea of connecting it to the instance singleton (where I suspect it belongs).

@acmorrow
Copy link
Member Author

acmorrow commented Apr 9, 2025

fine with merging this as is, but i wonder if going forward this may be past a certain threshold of ifdef proliferation where we want to split sources into signal_manager_win32.cpp and signal_manager_posix.cpp

I totally agree. I'm going to file an issue to revisit the signal handler infrastructure, and note both this idea and the idea of connecting it to the instance singleton (where I suspect it belongs).

Filed https://viam.atlassian.net/browse/RSDK-10417

@acmorrow acmorrow merged commit fddae77 into viamrobotics:main Apr 9, 2025
4 checks passed
@acmorrow acmorrow deleted the windows branch April 9, 2025 14:52
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.

2 participants