Mavsdk: Health not clearing cache

[douglaswsilva]

Telemetry Health Subscription does not clear cache when aircraft is disconnected, so if you turn aircraft off and on again, it will say it has home position and global position even though it has not locked GPS yet. This will prevent vehicle form taking off.

[JonasVautherin]

@julianoes: how does a plugin know the vehicle is disconnected?

[julianoes]

Good find @douglaswsilva.

@JonasVautherin we should reset in here:
https://github.com/Dronecode/DronecodeSDK/blob/0e6a1aeb6ce3132e04ee922720dfc875ea138bf1/plugins/telemetry/telemetry_impl.cpp#L126

[irsdkv]

I have a same problem with param module.
Cache used during all backend runtime although UAV was rebooted or params values was changed from the other source (QGC or PX4 console, for example).

Also I think that solution should have API for gRPC request like "CacheResetRequest" for modules which use caches.

What are you think about this?

[irsdkv]

Or may be it would be ideal if user also will have ability to turn caching off/on for module

[JonasVautherin]

Also I think that solution should have API for gRPC request like "CacheResetRequest" for modules which use caches.

My opinion about caches is that it should be transparent. If the user needs to be aware of the fact there is a cache, then it is a bad cache :sweat_smile:.

Examples:

• Caching a "battery status" is fine, so that users receive the last position as soon as they subscribe to it instead of waiting until the next battery state (which may be a few seconds away).
• Caching parameters is fine if the autopilot broadcasts the new values whenever they change. It is not fine if MAVSDK has no way to know that the parameter was changed, and therefore we should just always request the param, with the caveat that sometimes this may time out.
• Caching the mission is not a good idea unless we have a way to know that the mission got updated on the drone.

If we can't have a real (transparent) cache, let's not have any cache, and push that as a MAVLink issue that needs to be fixed at the autopilot level (i.e. the autopilot should tell us when its state change).

What do you think?

[irsdkv]

This is good clarification.
I dont know about specific use cases for Battery Status and Mission, but I think that Parameters module has no reasons to use cache:

• Seems that MAVSDK has no way to know that parameter was changed
• I think that use cases of MAVSDK does not include real necessity of fast frequency parameters values polling (which may require cache using)

[hamishwillee]

Caching parameters is fine if the autopilot broadcasts the new values whenever they change. It is not fine if MAVSDK has no way to know that the parameter was changed, and therefore we should just always request the param, with the caveat that sometimes this may time out.

Components are expected to _broadcast_ updated param values so that all interested systems can synchronize their internal record of the parameter list. Note however that this is not reliable in lossy environments. The actual requestor of the param change is fine because it is expected to re-set the param if it does not get a PARAM_VALUE. However systems that are just monitoring for PARAM_VALUE will fall out of sync if they miss a message.

This is not "resolvable" with the current protocol definition. It can be somewhat mitigated if the autopilot emits the PARAM_VALUE multiple times when it is changed.

[JonasVautherin]

That's interesting. But then that's not what @irsdkv is observing, somehow :thinking:.

[julianoes]

I see two things that might go wrong:

1. MAVSDK doesn't update if PX4 is disconnected and reconnected again because the param is still in the cache even though it has been updated in the meantime.
2. MAVSDK is not connected when the param update happens and misses the broadcast.

[irsdkv]

@hamishwillee
I'm checked PX4 Firmware to find out what exactly occurs on parameter changes.
This ORB topic really provide the ability to resend new parameter values, but in MAVLink and MAVSDK we have not way to receive notify on param change. I admit that I'm wrong, but required large implementations in PX4 and MAVSDK both to notify if value of parameter in cache list was updated.
Also reset parameter value during reboot still problem for that.

Notify for PR

[julianoes]

but in MAVLink and MAVSDK we have not way to receive notify on param change

That's true. We should always listen for params, and update the cache if we get one, right?

Also reset parameter value during reboot still problem for that.

Right.

[irsdkv]

We should always listen for params, and update the cache if we get one, right?

Yes, we should listen parameter_update topic and update the cache on every topic's message or every PARAM_VALUE request.
instance can be used to check if MAVSDK should update cache, but MAVLink protocol don't proceed that ability. Also seems overhead to update all cache on any parameter's value update.
MAVLink docs say that we haven't reliable way to keep up values consistence at this time.

[hamishwillee]

My two bits .... you should follow the spec.

• Every time you connect to a system you should get all its params. It is not explicitly noted, but if you disconnect and reconnect, then you should still get all the params again.
• A component should broadcast PARAM_VALUE every time the value is set (even if it doesn't change). MAVSDK should maintain a cache of params, and update them whenever a PARAM_VALUE is received.

It isn't perfect, but any other alternative is non-compliant with the spec (and likely to cause more problems in practice).

[irsdkv]

Every time you connect to a system you should get all its params. It is not explicitly noted, but if you disconnect and reconnect, then you should still get all the params again.

How can we check that UAV has been rebooted? May be I will try to implement it in MAVSDK.

A component should broadcast PARAM_VALUE every time the value is set (even if it doesn't change). MAVSDK should maintain a cache of params, and update them whenever a PARAM_VALUE is received.

Is that implemented in PX4 now?

Non-compliant with the spec.

Sorry, I cant find any mention of parameters cache in the spec. But i founded spec says that

param_count may be monitored. If this changes the parameter set should be re-sychronised

(Link)

[irsdkv]

Let imagine that situation:

1. During all session (which may last several hours) we received 50 parameters.
2. All 50 parameters on caching now
3. MAVSDK received that param_count has been changed
4. MAVSDK update all cache (50 parameters).
5. But now for us there is no need for all 50 parameters or at this time we no longer need any parameters values

I think that caching looks like radio channel overload in that case

[julianoes]

Every time you connect to a system you should get all its params. It is not explicitly noted, but if you disconnect and reconnect, then you should still get all the params again.

I think this "spec" comes from the way how a ground station like QGC generally works. I don't think it makes sense. As @irsdkv noted this mostly mean a lot of traffic for not much use. What MAVSDK does today is to only get the params it knows and requires because it has absolutely no use for the other few hundred params.

A component should broadcast PARAM_VALUE every time the value is set (even if it doesn't change). MAVSDK should maintain a cache of params, and update them whenever a PARAM_VALUE is received.

MAVSDK should update the cache for used/known param when it receives PARAM_VALUE as said above.

[irsdkv]

Let imagine that situation:

1. During all session (which may last several hours) we received 50 parameters.
2. All 50 parameters on caching now
3. MAVSDK received that param_count has been changed
4. MAVSDK update all cache (50 parameters).
5. But now for us there is no need for all 50 parameters or at this time we no longer need any parameters values

I think that caching looks like radio channel overload in that case

UPD. Sorry, I was wrong about param_count. My bad, I was confused with it and parameter_update.

So this example should use parameter_update, which does not implemented in MAVLink protocol.

[hamishwillee]

I think this "spec" comes from the way how a ground station like QGC generally works. I don't think it makes sense.

1. What if someone wishes to use MAVSDK as the base for a GCS?
2. If the SDK is supposed to represent the best implementation of the protocol, then it should follow the spec, or we should attempt to revise the spec to make sensible statements about what different types of users should do.

Anyway, I do understand your point and largely agree. Just playing devils advocate.

[julianoes]

What if someone wishes to use MAVSDK as the base for a GCS?

In order to make use of all the params you also need the xml file describing all params, otherwise a list of cryptic param names is not very helpful anyway.

The alternative is a MAVSDK function which maps to a specific parameter. For instance we have a method set_takeoff_altitude which then maps to MIS_TAKEOFF_ALT.

or we should attempt to revise the spec to make sensible statements about what different types of users should do.

I think that would make sense.

[JonasVautherin]

So what should we do? @hamishwillee seems to imply that #847 is not compliant, but I'm not sure why. I guess I would not expect GetIntParams to send me updates.

To me, GetIntParam should always give me the right value, in which case not having a cache is the best way to achieve that (since I can always fall out of sync otherwise).

Then we could imagine having another interface for observing param values (e.g. if I write a GCS that has a UI showing the parameters). There I may want to SubscribeIntParam (for one specific param) or even SubscribeParams (for all/a subset of them). And even in that case, one could argue that MAVSDK would not need to have a cache and could delegate that to the client (even though having a cache would be nice).

@hamishwillee: does that make sense, or do you actually think that not maintaining a cache for GetIntParam is non-compliant?

[hamishwillee]

@JonasVautherin

1. I was wrong - removing the caching is still compliant. I have fixed up the wording in https://github.com/mavlink/mavlink-devguide/pull/204 to make this more obvious.
2. I do not think we should cache parameters, but I do think we should make it possible for a user to build such a cache if they want, noting it will have the limitations of the underlying protocol.

The point of _any_ cache is to have a local copy that you can access quickly (and in the MAVLink case, to also reduce traffic on the link by not having to reload data). If you choose to use a cache then either you have mechanisms to guarantee the cache is up to date, or you accept that it is OK for it to not be up to date. Otherwise you don't cache.

For parameters we could cache them because there isn't AFAIK "much cost of getting it wrong" (they very rarely change, and even if they do that mostly only matters if you're rendering a UI dependent on their values). On the other hand, most MAVSDK use cases don't rely on us needing to read parameters, or at least very many, and they don't rely on that process being very timely. So on balance, easier just not to cache.

For other values I do think caching makes sense, but we do need to think about this some more. I don't care if my battery level info is a few seconds old, but I may care if it is 10 minutes old. For altitude, I do care if it is more than a few seconds old. Upshot, maybe a cache needs to invalidate, or at least include a timestamp for some values.

[JonasVautherin]

Ok, I merged #847. Still the cache issue related to health is not solved, so I keep this issue open.

[julianoes]

Note to self. Instead of using params for health checks we should switch to checking the flags of https://mavlink.io/en/messages/common.html#SYS_STATUS.