Shearater: Remove timesync for Predator.

[mikeller]

Remove the timesync function from the code for the Predator - testing has shown that this function is not supported by this model.

[jefdriesen]

What response do you get from the Predator? A NAK (which is handled in the code and results in DC_STATUS_UNSUPPORTED), or something else?

Note that the predator backend does not only support the Predator, but also some of the later models like the Petrel. That's certainly not the optimal choice for those models, but it's possible and also supported.

[mikeller]

What response do you get from the Predator? A NAK (which is handled in the code and results in DC_STATUS_UNSUPPORTED), or something else?

Note that the predator backend does not only support the Predator, but also some of the later models like the Petrel. That's certainly not the optimal choice for those models, but it's possible and also supported.

I frankly did not check the response - just that the time was not updated. I had to borrow a Predator from a friend, so my access for testing is limited.

What are the use cases for users with more recent Shearwater models to use the Predator backend? Maybe support for this should just be dropped to avoid confusions and user questions?

[jefdriesen]

If the Predator supports the WDBI command, but not the time id, then I suspect we get back a NAK and the call will fail with DC_STATUS_UNSUPPORTED. Exactly as your change would do, except that for other models the call will now fail too. If the Predator doesn't support the WDBI command at all, then I'm not sure how it will respond.

When a user has a Petrel and also chooses a Petrel as the model, then the petrel backend will be selected. But sometimes users do choose the wrong model (for example because they don't know which model they have). This works because some of the newer models are backwards compatible and still support the older Predator protocol. Since it's the user that makes this (suboptimal) choice, you can't really drop support for this. Adding an extra check and refusing to continue would only result in more questions asked (e.g. why does downloading no longer work after upgrading?).

[mikeller]

Since it's the user that makes this (suboptimal) choice, you can't really drop support for this.

We could just tell them that they should be using the correct backend for their dive computer.

Adding an extra check and refusing to continue would only result in more questions asked (e.g. why does downloading no longer work after upgrading?).

Not sure I follow you there - what will no longer work after they switch to the correct backend for their computer?

Different question: Have you tested that time synchronisation actually works with newer dive computer models than the Predator when using this backend? Because I haven't, and I am not keen to invest time to test and maintain this, as we already have a tested and usable solution for these models.

[jefdriesen]

We could just tell them that they should be using the correct backend for their dive computer.

How do you tell them? Libdivecomputer doesn't really have a user interface to tell the user anything, except for returning an error code like DC_STATUS_UNSUPPORTED.

BTW, I think you would be surprised how many users don't know the exact model they have. I see such mistakes pretty often.

Adding an extra check and refusing to continue would only result in more questions asked (e.g. why does downloading no longer work after upgrading?).

Not sure I follow you there - what will no longer work after they switch to the correct backend for their computer?

You wanted to drop support for the newer models from the predator backend. That would require adding a check like this:

if (model != PREDATOR)
    return DC_STATUS_UNSUPPORTED;

But with the predator protocol, we first have to download the entire memory dump before we can inspect the model number. So that means that after a successful download, you want to discard the downloaded data, and tell the user downloading isn't supported?

Different question: Have you tested that time synchronisation actually works with newer dive computer models than the Predator when using this backend? Because I haven't, and I am not keen to invest time to test and maintain this, as we already have a tested and usable solution for these models.

I no longer have a Petrel available myself. But even without any testing I can tell you that if the timesync in the petrel backend works for the Petrel, then the one from the predator backend will work too, because both backends call the same underlying function. The difference between the predator and petrel backend is mainly about the implementation of the foreach function. The rest of the code is shared between the two.

[mikeller]

How do you tell them? Libdivecomputer doesn't really have a user interface to tell the user anything, except for returning an error code like DC_STATUS_UNSUPPORTED.

We could simply return a different status for 'wrong dive computer model selected'. Or return an error message string with details.

But with the predator protocol, we first have to download the entire memory dump before we can inspect the model number.

That sounds very inefficient - so wouldn't users with a newer dive computer than the Predator be better off if they switched to using the correct backend, which then will only download newer dives?

The difference between the predator and petrel backend is mainly about the implementation of the foreach function. The rest of the code is shared between the two.

That means that this could most likely be simplified for the user by folding the two Shearwater backends into one - try the 'Petrel' way to read the model first, and if this fails fall back to using the 'Predator' way...

[jefdriesen]

We could simply return a different status for 'wrong dive computer model selected'. Or return an error message string with details.

I don't think this is a good idea. It breaks existing functionality and it's not really about a wrong choice, but a less optimal one. If we want to address this somehow, then trying to implement some auto-detection is probably a better option .

That sounds very inefficient - so wouldn't users with a newer dive computer than the Predator be better off if they switched to using the correct backend, which then will only download newer dives?

Of course.

Note: If I remember correctly, it's only the Petrel (and maybe some other older models?) that supports both the old predator and the more efficient petrel protocol. This was mainly for backwards compatibility with existing applications that did not yet support the newer protocol. But I believe it also depends on the firmware versions. It's possible that some Petrels with a (very) old firmware do not support the newer petrel protocol at all. In that case they have to choose the predator protocol.

That means that this could most likely be simplified for the user by folding the two Shearwater backends into one - try the 'Petrel' way to read the model first, and if this fails fall back to using the 'Predator' way...

That's something that might be possible. I already considered this in the past. There are two issues with this:

First, I have no idea if the RDBI commands that are used in the petrel backend to identify the model number, serial number, etc are supported by the older Predator. I suspect it does support the RDBI command itself, but not necessary all the IDs. I no longer have the hardware to test this, so I never really investigated this possibility further. If the RDBI command is not supported, then this gets more complicated. The petrel backend already has some format detection:

https://github.com/libdivecomputer/libdivecomputer/blob/master/src/shearwater_petrel.c#L206-L228

The second issue is that the predator and petrel backend have a slightly different data format. In the parser, the distinction between the two is made based on the backend type. If the two backends are merged, then this approach no longer works and we need to come up with an alternative.

So if we want to give this a try, we first need someone with a Predator (and preferably also a Petrel) to test a few things for us.