Open-Agriculture / AgIsoStack-plus-plus

AgIsoStack++ is the completely free open-source C++ ISOBUS library for everyone
https://agisostack.com/
MIT License
170 stars 40 forks source link

Update supported version in working set maintenance message #374

Closed GwnDaan closed 7 months ago

GwnDaan commented 7 months ago

Currently we ask the application to provide the "supported" VT version for a pool. This is not what the standard want us to provide. It instead requires us to set the VT version in the maintenance message to the version of the standard that the application is designed to.

I think the correct thing to do is provide the standard version the application is designed to comply with. Since the stack is basically the one doing communication defined in the standard, that means that we can put it to a constant version. Namely we aim to be complaint with VT version 6, i.e. ISO11783-6:2018.

Reference to applicable code: https://github.com/Open-Agriculture/AgIsoStack-plus-plus/blob/495195584f1a8a30e9474d7aa8ded6657dcddc3e/isobus/src/isobus_virtual_terminal_client.cpp#L1767-L1819

ad3154 commented 7 months ago

We will have to monitor what happens when we do this. Although I agree with it, I have seen some VT servers do some pretty funky stuff, and we should recall that clients are required to change their behavior based on the server version, so it may be less obvious to consumers now that version is something they need to be thinking about. We should be sure that our logging statements are high quality for situations where the server version < client.

GwnDaan commented 7 months ago

Yeah I see your concerns, and they are all valid ones for sure. Do you reckon we should wait with this change? I do think it's a bit more complicated than the phrase I initially put:

Since the stack is basically the one doing communication defined in the standard, that means that we can put it to a constant version. Namely we aim to be complaint with VT version 6, i.e. ISO11783-6:2018.

But I think if we implement the standard more thoroughly in the coming months, we should be fully backwards compatible? Also I'm sure if I understand how this change can mess up a connection with a VT server (if it follows the standard haha)?

ad3154 commented 7 months ago

if it follows the standard

I'm not going to name any names... but I have seen some... things in my days... It is not reasonable to try to cover non-conformant terminals probably.

we should be fully backwards compatible

The main thing we cannot control is what the user puts in the object pool, and what functions they call, both of which may result in non-conformant behavior, so we may need to add some additional validation as we implement the standard more thoroughly in the coming months to minimize this possibility?

ad3154 commented 7 months ago

It is not reasonable to try to cover non-conformant terminals probably

(Except AgIsoVirtualTerminal hehe)

ad3154 commented 7 months ago

I think it's OK to make the change now, but we'll then want to prioritize follow-up refactoring in the client for anything we're not conformant on, such as TAN processing for version 6 servers

GwnDaan commented 7 months ago

Yeah sounds good, let's make some issues to try to upgrade our compatibility

GwnDaan commented 7 months ago

[The main thing we cannot control is] what functions they[the application] calls

We could limit the outgoing messages based on the VT version connected right? E.g. we can make functions like send_lock_unlock_mask (Introduced in VT version 4) always return false if used when connected to a VT version 3 or prior. Or would that be undesirable?

GwnDaan commented 7 months ago

The main thing we cannot control is what the user puts in the object pool

Yeah I think it's also not our job to validate it within the stack. Probably nice to make an open-source tool to test conformance someday to provide a way of validating, but other than that we have to trust their good will. We can however require them to supply requirements for the pool, see #376, so they at least think about it...

ad3154 commented 7 months ago

always return false if used when connected to a VT version 3 or prior

This makes sense I think as long as we make a corresponding log statement, otherwise people may be confused