-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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: poll sensor_combined if estimator replay enabled #14734
Conversation
I'm not fond of the added dependency on the topic publication. |
Yes, it's a bit gross. Let's get some data across a few boards and see if it's justified. The reason this seems potentially compelling is it nicely schedules logger to run right after the bulk of the data is produced (sensor_combined, estimator, controllers). If you follow it through from each sensor_combined publication and everything downstream triggered as a result (estimator, attitude & position controllers) that's somewhere around 60% of a typical log (default settings). |
f29b675
to
18b3c40
Compare
Interesting discovery, the logging rate has increased drastically with this change. Looking at px4_fmu-v4 on the test rack. INFO [logger] Wrote 1.91 MiB (avg 40.02 KiB/s) <- master
INFO [logger] Wrote 35.12 MiB (avg 189.89 KiB/s) <- PR A portion of this is |
Currently logger disables the interval for all topics if a polling topic if enabled, so that explains the weird behavior above. This is what I'm seeing now. INFO [logger] Wrote 1.91 MiB (avg 40.02 KiB/s) <- master
INFO [logger] Wrote 4.94 MiB (avg 47.06 KiB/s) <- PR Logger CPU usage increase. PID COMMAND CPU(ms) CPU(%) USED/STACK PRIO(BASE) STATE FD
586 logger 2957 3.458 2832/ 3644 233 (233) READY 4 <- master
586 logger 2652 5.115 2832/ 3644 233 (233) w:sem 5 <- PR The other thing that stood out to me is that although the logging rate increased, the dropouts were shorter. I'm not sure if that's meaningful over a ~2 minute log. Master ulog info
PR ulog info
The throttled topics also appear to be much closer to their configured intervals.
Note the default IMU integration period changed (4000 us -> 2500 us) to align with the new ecl/EKF filter update period (10 ms), so either way we'll need to do something with logger (polling or increase default rate). Later I think we could effectively bring down the ekf2 replay logging requirements considerably by only logging the downsampled IMU (100 Hz). I'll revisit that soon during multi-EKF development where ekf replay logging by default is going to become quite problematic. |
18b3c40
to
a0268bc
Compare
Related: #14759 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logger's main loop is quite efficient, so increasing the rate should be viable.
I'm only ok with polling as default config if we use a short timeout of around 3ms.
|
||
uint16_t interval_ms = sub.interval_ms; | ||
|
||
if (_polling_topic_meta && (get_orb_meta(sub.id) == _polling_topic_meta)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still breaks replay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on that? What would you suggest given the overall goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rate limitation of the topics uses the absolute time which does not match the replay time.
We need to use the time abstraction for replay to resolve this cleanly.
The quick-and-dirty way would be to check _replay_file_name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll put this on hold depending on the outcome of #14759.
#14759 has merged with a default IMU integration rate of 200 Hz, which works well with the current logger interval. |
Noticed primarily in #14676 where the default IMU integration period changes (sensor_combined at 400 Hz instead of 250 Hz). This should help things even with existing setups and 250 Hz IMU.