bring in class LogListHelper into MavlinkLogHandler #14452
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bkueng, can you review? |
bkueng
reviewed
May 11, 2020
| @@ -105,15 +65,42 @@ class MavlinkLogHandler | |||
| unsigned get_size(); | |||
|
|
|||
| private: | |||
| enum class LOG_HANDLER_STATE { | |||
| INACTIVE, | |||
There was a problem hiding this comment.
Can you clarify the difference between idle and inactive? Maybe use more descriptive names?
There was a problem hiding this comment.
@bkueng I tried not to change the logic too much, not for now. so INACTIVE is similar to the case that the LogListHelper has not been created.
I added notes, and I know its a bit confusing, but without wider changes I don't want to change it much more.
this is in order to avoid dynamic allocation of LogListHelper when downloading logs. LOG_HANDLER_STATE::INACTIVE state is the state to indicate that the behavior of LogListHelper not exists
fixed function name _close_and_ulink_files() to _close_and_unlink_files()
|
@bkueng thanks for the review, I have made changes. and rebased. |
BazookaJoe1900
force-pushed
the
pr-mavlink_log_remove_helper
branch
from
661d538
to
1fdc8a8
Compare
bkueng
changed the title
bkueng
reviewed
May 14, 2020
|
@bkueng Changes done according to your notes, thanks. |
bkueng
approved these changes
May 25, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is followup to 'return with error from _log_request_list() in case of failed allocation #14451'
Describe problem solved by this pull request
The class LogListHelper created and destroyed dynamically during logs download process. which mean some
newanddeletebuilt in in MavlinkLogHandler code. the existence of the class then checked, to decide the state of the downloader.This PR remove the dynamic allocation. to ensure enough memory on creation of MavlinkLogHandler (meaning, creation of mavlink instance)
Describe your solution
The class LogListHelper removed, all its functions and variants moved into MavlinkLogHandler and created when MavlinkLogHandler allocated.
LOG_HANDLER_STATE::INACTIVE state is the state to indicate that the behavior as if LogListHelper not exists
Describe possible alternatives
question:
now, on every creation of mavlink a MavlinkLogHandler allocated, with an about extra 150 bytes.
most of this memory is _current_log_filename[128].
661d538#diff-e56b8d1bc66526d6f124b0bf662f02fdR105
is that acceptable?
it can be done that only _current_log_filename[] will be malloc dynamically, only if process of download is initiated.
Test data / coverage
tested proper behavior on FMUv5, download speed hasn't been changed and still ~330kBps
Additional context
more improvements can be done to the module. the commits that done keep the exact same behavior. and according to the response about this change, I can do more small/big fix on the process of downloading logs.