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

logger: always log can_interface_status #24071

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dakejahl
Copy link
Contributor

@dakejahl dakejahl commented Dec 3, 2024

There's a caveat with LoggedTopics::add_optional_topic, it will fail to log topics that don't exist at the time of the logger instantiation. So any sensor that publishes later (CAN nodes for example) won't be logged. Namely the newish CanInterfaceStatus message, the status of the CAN interfaces is not logged since a CAN node will typically take longer to boot than the FC does.

@dakejahl dakejahl requested a review from dagar December 3, 2024 23:57
@mrpollo mrpollo force-pushed the pr-always_log_can_interface_status branch from b312338 to 39708cb Compare December 11, 2024 19:09
@oystub
Copy link
Contributor

oystub commented Jan 10, 2025

Thanks for this, this is totally my bad. I did a fix for this in our fork, as I mentioned here #22873. but completely forgot to follow it up in the upstream. I got a bit "sniped" by the mutex lock uavcan_status issues which I never had time to resolve.

We should definitely get this PR merged as a bugfix :)

@dakejahl I think we should do

	add_topic_multi("can_interface_status", 100, 2);

This is to:

  • Log at 10 Hz as intended, instead of 100
  • There is one topic for each CAN bus, and many targets have two buses. Therefore, we should log at least two.

It is not very nice to hard-code the "2". I think the number of UAVCAN interfaces is only defined in the Kconfig for the uavcan driver here:
https://github.com/PX4/PX4-Autopilot/blob/a231fafafadf92ec76022e6afdfcb5aeb7e4ae90/src/drivers/uavcan/Kconfig#L96C1-L101C37 I don't think we can refer to that directly without introducing ugly couplings between the modules. Any suggestions for doing this nicely?

@dakejahl
Copy link
Contributor Author

@oystub thanks for the suggestion! See latest commit

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