Hi, I just noticed a pretty strange behaviour:
TelemetryServer.publish_xxx() calls make two things:
- cache provided telemetry message;
- send telemetry message.
If I call Telemetry.set_rate_xxx() (same telemetry message of course) the remote system will start sending xxx telemetry messages at the requested rate. If I need to refresh the data provided by this mechanism I need to call again on publish_xxx() … but this will trigger another (unwanted) delivery of a telemetry message. On a system that frequently updates the telemetry data this will generate and unwanted flow of telemetry messages.
I’m missing something?
There’s another way to update data without forcing telemetry dispatch?
Let me answer with a question: how bad is that?
I mean, you probably won’t change the rate very often, and when you do, you will indeed get an extra update. But do you care?
IMHO is bad and, of course in my opinion, this’s also meaningless:
- why I should set up a timer based telemetry service if each time the updated data is alread sent?
You could just rely on the data published along with update without using timed telemetry;
- think about IMU or other high frequency sensors output: these data are updated very frequently, probably much more frequently than your standard telemetry update rate;
- for sake of bandwidth you definetly don’t want to waste time and band sending data you already sent;
- if you opted for a timer based telemetry you want packets at a defined rate then with updates you got them at a different, higher, frequency why?
Oh I get it now. Given that at init() time, it starts sending the updates, it should probably not send them at all on publish_xxx(), e.g. here. Instead it should just add_msg_cache and let it go with the next update.
It seems like a bug to me, thanks for raising it!
Would you mind trying a fix? I guess it could just return TelemetryServer::Result::Success instead of calling send_message.
Yes I fixed it that way on my own code … just tought it was too simple.
Haha no it seems correct to me 
. Could you open a PR, so we can have a closer look at the code there?
And thanks again 
no prob… need to make some cleanup but I will…
1 Like
I have a doubt about updating source code with proposed fix, at the moment TelemetryServer works for both explicit and timed based telemetry (with the obvious bug in time based telemetry) removing message send in publish_xxx() methods will fix only timed telemetry but will disrupt explicit telemetry.
We have two solution scenarios:
- keep
publish_xxx() methods AS IS and defining a new method cache_xxx() that will only store xxx messages in cache
- modify/overload method
publish_xxx() adding a boolean parameter cache_only (default value false)
In both cases we need to modify telemetry_server.proto file.
I’m trying with solution 1 since is the only one I’m able to get working.
Since I’m not very expert in rpc mappings and I can’t find an example of .proto file with methods signature as for solution 2 I need some help to learn the way… could someone help/address me?
Best regards,
Mike
Wouldn’t cache_xxx() (solution 1) be similar to publish_xxx() in the proto file?
nope. publish_xxx() will cache and then send message while cache_xxx() will only store data in cache that will be sent by the internal TelemetryServer timer.
In this way users that wish to use a subscription based telemetry can use cache_xxx() metods to provide asynchronously data to the telemetry server that will send it at the requeste rate (assuming they first subscribed it trough set_rate_xxx()), while users that wish to send telemetry on a different fashion could still use publish_xxx() methods as they probably did before.
Sure, but in terms of the proto definition, I think it’s similar. Then you have to implement it differently in telemetry_server_impl.cpp.
oh yes is quite similar from proto point of view.
my concern was about solution 2 method signature… I cannot figure how to define it in proto.
BTW I defined cache_xxx() methods in proto and telemetry_server_impl.cpp and it works fine.
1 Like
Can you open a PR with those changes, so we can continue the discussion there?
