Report an error when the VT server can't use an object pool

[sujandumaru]

Describe your changes

Related to #707 but does not close it. #707 is about catching invalid values like an InputBoolean with value 24, and that check is not in this PR. This PR fixes the part before it: the server telling the client when a pool is not usable.

The server already has everything it needs to report a bad object pool, it just doesn't use it. In VirtualTerminalServer::update() the Fail branch called send_end_of_object_pool_response(true, ...), so a pool which failed to parse went back to the client with byte 2 and byte 7 both zero. In ISO 11783-6 that is the encoding for "pool accepted". So the client connects, gets an empty object tree, and shows nothing. It looks like the VT took the pool, which makes this confusing to debug.

While I was there I found a second one which is worse. A pool can parse fine and still have no Working Set object. That branch dereferenced get_working_set_object() without a check, and get_object_by_id() ends in std::map::operator[], which inserts a null shared_ptr for a missing key. So the server segfaults, and a client can do it with a one object pool.

Changes:

• Both cases now send a real error: byte 2 bit 0 (error in the object pool) and byte 7 bit 2 (any other error). Added ObjectPoolErrorBit so the call sites don't pass bare numbers.
• A pool with no working set object is rejected instead of crashing, and does not become the active working set.
• Renamed the errorCodes parameter of send_end_of_object_pool_response() to objectPoolErrorCodes. It goes into byte 7, but in the client errorCodes means byte 2. Same name for two bytes is what made this easy to call wrong.
• print_objectpool_error() didn't decode byte 2 bit 0, the one bit the server sets now, so it printed the header line and then nothing. Added it.
• initialize() never set initialized, so the destructor's if (initialized) never removed the rx callback. Every destroyed server left a dangling process_rx_message registration behind. I only hit it because two server tests now share one binary, but it is
  a use after free in any app which destroys a server.

The first commit is separate. Container is allowed as a child of data masks, alarm masks and the version 2 auxiliary objects, but the parser rejected it there, so a conformant pool could fail to load for no reason. Cross-checked with allowed_object_relationships.rs in AgIsoTerminalDesigner.

How has this been tested?

Three new tests. Two push a bad pool through the server over VirtualCAN and check the error bits which actually go out on the wire, one for a pool that fails to parse and one for a pool with no working set object. The third covers the Container parent types.

cmake -S . -B build -DBUILD_TESTING=ON -DCAN_DRIVER=SocketCAN
cmake --build build
ctest --test-dir build -R 'VirtualTerminalServerMessagingTest|VIRTUAL_TERMINAL_OBJECT_TESTS' --output-on-failure
ctest --test-dir build --output-on-failure

[GwnDaan]

Looks good overall. One thing I would change: I don't think this should Fixes #707, since that issue is specifically about validating invalid IOP values such as an InputBoolean value of 24. That validation is still not added here, so the issue would be closed while the original problem remains.

I'd remove the Fixes #707 reference and keep this PR focused on the VT server error handling fixes. And add the detection of invalid IOP attribute values (specifically an InputBoolean value like 24 in #707) inside another PR

[sujandumaru]

Good point, I agree. Missed the checking part. I changed the wording on this.

I'll open another PR for the InputBoolean value check. One thing I noticed while looking at it: right now get_is_valid() for InputBoolean, InputNumber and InputList only checks the child and reference types, not the values. Should we make the value check for other types as well? For example InputNumber value against its min and max, or InputList value against the number of items. And should an invalid value reject the whole pool with an error response, or only log a warning? I am not sure how strict we want to be here.

[sujandumaru]

I checked the standard before starting the InputBoolean PR, and I think I was wrong in my last comment. In ISO 11783-6 B.8.2 the Input Boolean value range is 0, 1 to 255. From VT version 4, any value > 0 is TRUE and 0 is FALSE, and AgIsoVirtualTerminal already handles it like this. So a value of 24 is valid. My guess is the Kverneland Tellus in AgOpenGPS-Official/AOG-TaskController#67 refused it because it may follow the old version 3 range but I don't think we should reject the value of 24 from the Stack for now. And we did fix the .iop file itself on TC side.

I would still like to keep this PR. When a pool fails to parse for any other reason, the server still answers the End of Object Pool with "no error", so the client thinks its pool was accepted. This PR makes it send the error bits and the faulting object ID instead.

What do you think about this @GwnDaan @gunicsba?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
66.7% Coverage on New Code
2.1% Duplication on New Code

See analysis details on SonarQube Cloud

[gunicsba]

I have a feeling that most VT-s I touched are working at V3 levels.

With that being said should we start to add bunch of IF / ELSE for all these things depending on the version? (I.e. refuse the iop becuase we enforce V3 or we let it slip because V4 allows it?)

If we're at V3 levels we should enforce V3 compatibility (but that'll be a pain to manage in the long run)

I really don't think there's a good solution here. The error reporting is definitely needed though.

[sujandumaru]

I agree, I don't think there is a clean solution for this. Enforcing different behavior based on VT version can be exhausting as there are a lot of other behaviors which are different between versions but it can also build strong foundation. And it will require to keep track of version attribute on most behavior.

[GwnDaan]

I think we should separate VT client and VT server behavior here:

• For the VT client, PR VT Client: Add a way to select VT object pool(s) to use based on VT server version #607 already gives the application the right mechanism: detect the VT version, then supply the appropriate prevalidated pool.
• For the VT server, I think we should validate according to the VT version the server itself advertises. Since newer VT versions are backwards compatible, a VT6 server should accept valid older pools, but it should apply VT6 semantics when interpreting them.
  So for InputBoolean, if the server advertises VT4+, a value like 24 is valid because 0 = FALSE and >0 = TRUE. I would not add checks based on the Working Set's reported version just to emulate older VT behavior.
  The server should still reject genuinely invalid pools, bad references, unsupported objects/attributes, missing required structures, etc

[sujandumaru]

Thanks @GwnDaan, that makes the split clear. I will not open the separate InputBoolean PR for now. Once #607 merges, it would be easy for server to get correct pool and reduce this kind of error. I will merge this PR so that the server can reject wrong pools.