bluerobotics / ping-firmware

Repository for binary files associated with ping devices
5 stars 8 forks source link

Fix `distance` messages to match spec #27

Closed ES-Alexander closed 4 weeks ago

ES-Alexander commented 11 months ago

Currently the ping_number field is left at 0 for get_distance, ~and is set to the request number for get_profile, whereas the docs say it's the pulse/measurement count, which should continue counting even while no messages are being requested.~

EDIT: re-tested with Ping-V3.29.0_auto and the number seems to be correct, just not currently set in the distance messages. Not sure whether I originally tested an earlier firmware version, or if I was just mistaken about it being set to the request number 🤷‍♂️

This seems like it should be quite straightforward to implement with a global count variable that's incremented whenever the device pings, and gets accessed when packing the messages that send it.

Issue raised in this forum post, and I've verified (using ping-python) ~both the "count profile requests instead of pings" behaviour and~ that the distance.ping_number field is consistently returned as 0 values.

mjsaenz commented 1 month ago

Hello, I am the OP of the forum post and have recently returned to the project for which I was working with the Ping2 echosounder. Has any progress been made on this issue?

ES-Alexander commented 1 month ago

Hi @mjsaenz,

This hasn't had any updates since the issue was raised, but I've brought it up again internally and it will hopefully get fixed within the next couple of weeks.


@patrickelectric to reiterate my clarifications from the meeting:

  1. The distance ~and profile~ messages sent by the ping1d firmware are incorrectly populating the ping_number field
    • I presume this is also true for the ping2 firmware, but I haven't confirmed
  2. The ping_number should be tracked internally, and incremented every time a ping is performed ~(NOT only when a profile message is requested/sent)~ (already seems to be the case)
  3. The current value should be included when a relevant (profile/distance) message is sent
ES-Alexander commented 1 month ago

@patrickelectric added the ping_number variable population to the distance message, which can be tested with this firmware: Ping2-V1.0.1_beta.hex.zip

We'll upload a version properly to the repository soon.

mjsaenz commented 1 month ago

Great to hear! How do we update the Ping2's firmware once the new version is pushed?

mjsaenz commented 4 weeks ago

Hey Blue Robotics team, was wondering if there were any updates on this front?

ES-Alexander commented 4 weeks ago

How do we update the Ping2's firmware once the new version is pushed?

Ping Viewer needs an update before it will properly differentiate between the original Ping Sonar and the newer Ping2 devices, which is relevant for auto-filtering the appropriate firmwares.

That said, the control PCB is the same between the two device versions, so you can flash on the test firmware (from my earlier comment) manually via the flashing functionality that's already in Ping Viewer - the lack of an official release shouldn't stop you from using the corrected firmware; a release will just make it easier to find / know about :-)

... was wondering if there were any updates on this front?

Not that I'm aware of, but I've asked about it again internally to see whether we can get that more official release sorted out.

ES-Alexander commented 4 weeks ago

Alright, we've now included the updated firmware, and created a new Ping Viewer release that properly supports the Ping2 🙂

Thanks @patrickelectric! 😃

patrickelectric commented 4 weeks ago

@mjsaenz let me know if this fix your issue

mjsaenz commented 3 weeks ago

Hi @patrickelectric, With firmware version 3.29, installed using the new Ping Viewer release @ES-Alexander shared above, the ping_number field still does not increment when calling get_distance(), though it does increment when calling get_profile(). After flashing the beta firmware, the ping_number field increments properly for calling get_distance() and get_profile().