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

Remove caching in params #847

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Remove caching in params #847

merged 3 commits into from
Sep 12, 2019

Conversation

irsdkv
Copy link
Collaborator

@irsdkv irsdkv commented Aug 27, 2019 •
edited
Loading

Due to this conversation:

  • Seems that MAVSDK has no way to know that parameter was changed on UAV after reboot or using console or QGC, for example
  • Use cases of MAVSDK probably does not include real necessity of fast frequency parameters values polling (which may require cache using)

@julianoes julianoes self-requested a review August 28, 2019 06:45
julianoes
julianoes previously approved these changes Aug 28, 2019
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

I suppose this makes sense. It will cause more delay for some calls but that's probably ok if instead the data is actually up to date.

@julianoes
Copy link
Collaborator

Closes #736.

@julianoes julianoes assigned julianoes and unassigned julianoes Aug 28, 2019
@JonasVautherin
Copy link
Collaborator

@julianoes: Does that really solve #736? The health cache issue comes from the telemetry plugin, right?

Also @hamishwillee mentioned there that the autopilot is supposed to broadcast updated params 😅.

@julianoes
Copy link
Collaborator

The health cache issue comes from the telemetry plugin, right?

Right, we need to verify this one. I'm not sure which values are not up-to-date.

JonasVautherin
JonasVautherin previously approved these changes Aug 28, 2019
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

LGTM

@irsdkv irsdkv mentioned this pull request Aug 28, 2019
Closed
@irsdkv
Copy link
Collaborator Author

irsdkv commented Sep 10, 2019

Somebody, can you please merge this PR if all ok?
I have not enough permissions for that :)

@JonasVautherin
Copy link
Collaborator

It's not completely clear to me from #736 if this is compliant or not 😅. Asking there.

JonasVautherin
JonasVautherin approved these changes Sep 12, 2019
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Thanks!

@JonasVautherin JonasVautherin merged commit 1405685 into mavlink:develop Sep 12, 2019
@JonasVautherin JonasVautherin changed the title Remove caching in params module Remove caching in params Oct 5, 2019
@irsdkv irsdkv deleted the pr-remove_params_cache branch October 10, 2019 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonasVautherin JonasVautherin JonasVautherin approved these changes

@julianoes julianoes julianoes left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@irsdkv @julianoes @JonasVautherin