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

Logger: dynamic number of subscriptions #13745

Merged
merged 4 commits into from
Dec 18, 2019
Merged

Logger: dynamic number of subscriptions #13745

merged 4 commits into from
Dec 18, 2019

Conversation

bkueng
Copy link
Member

@bkueng bkueng commented Dec 13, 2019

Fixes errors like Too many subscriptions, failed to add: sensor_selection 0.

See commits for details.
Memory usage is about the same, but depends now on the logging profile configuration.

@dagar
Copy link
Member

dagar commented Dec 14, 2019

From http://ci.px4.io:8080/blue/organizations/jenkins/PX4_misc%2FFirmware-compile/detail/PR-13745/1/pipeline:
image

We might as well drop the frame-larger-than warning, I don't think it's ever really helped.

@dagar dagar requested a review from a team December 14, 2019 16:03
@dagar dagar mentioned this pull request Dec 14, 2019
Merged
@dagar
Copy link
Member

dagar commented Dec 14, 2019

The SITL test failures looks real.

@bkueng
refactor logger: reduce required size of WrittenFormats
70461ac 
by keeping only recursive topics in there. The rest is checked by looking
at previous _subscriptions.

Reduces stack size requirements when increasing MAX_TOPICS_NUM.
@bkueng
Logger: dynamic number of subscriptions
69342ac 
Keep a fixed-size array of 250 requested topics on the stack, then allocate
an array with LoggerSubscription with the exact required size.
@bkueng bkueng force-pushed the logger_dynamic_subscriptions branch from 0981130 to b38cbf5 Compare December 16, 2019 09:39
@bkueng bkueng force-pushed the logger_dynamic_subscriptions branch from b38cbf5 to ac9db19 Compare December 16, 2019 09:43
@bkueng
Copy link
Member Author

bkueng commented Dec 16, 2019

We might as well drop the frame-larger-than warning, I don't think it's ever really helped.

Yeah, especially since it is a false positive.

The SITL test failures looks real.

Not sure what that was.

I moved the subscriptions into a separate class.

@dannyfpv
Copy link

dannyfpv commented Dec 16, 2019 •
edited by Tony3dr
Loading

Tested on Pixhawk 4 v5 f450
position mode: no issues
mission mode: no issues
altitude mode: no issues
stabilize mode: no issues

log:
https://review.px4.io/plot_app?log=ae40f57a-439c-4cc5-a9be-5eefb9f09629

@jorge789
Copy link

Tested on PixRacer V4:

Maximum wind 10.2 m/s

Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

- Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoin activate RTL and see landing behaviour.

Notes:
When the vehicle was landing, it did not descend vertically, drifted away with the wind and landed the vehicle manually

Log: https://review.px4.io/plot_app?log=719ab88b-d993-47c8-a9ca-eaf429d4577e

@bkueng
Copy link
Member Author

bkueng commented Dec 17, 2019

Thanks for testing.
@dannyfpv any reason why you set SDLOG_MODE to from boot until shutdown on the Pixhawk 4 v5 f450 vehicle?

@Tony3dr
Copy link

Tony3dr commented Dec 17, 2019

Thanks for testing.
@dannyfpv any reason why you set SDLOG_MODE to from boot until shutdown on the Pixhawk 4 v5 f450 vehicle?

@bkueng we were doing some bench test prior to the testing the PR. We forgot to disable the parameter.

@dagar dagar merged commit e10a4c5 into master Dec 18, 2019
@dagar dagar deleted the logger_dynamic_subscriptions branch December 18, 2019 05:23
@weekly-digest weekly-digest bot mentioned this pull request Dec 22, 2019
Closed
@Tony3dr Tony3dr mentioned this pull request Jan 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dagar dagar dagar approved these changes

Assignees
No one assigned
Labels
Admin: Enhancement (improvement) 💡
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bkueng @dagar @dannyfpv @jorge789 @Tony3dr