search

L-Acoustics / avdecc

A set of open source libraries for controlling AVB entities using the AVDECC (IEEE 1722.1) protocol compliant to Avnu Milan Specifications
GNU Lesser General Public License v3.0
90 stars 21 forks source link

dynamic_info responses in GET_DYNAMIC_INFO are not handled separately #141

Open Florob opened 11 months ago

Florob commented 11 months ago

As entities fill a GET_DYNAMIC_INFO responses dynamic_infos field as if each dynamic_info was a separate command, it would follow that controllers should also process the responses as such.

Currently the first error result will cause all processing to stop: https://github.com/L-Acoustics/avdecc/blob/55ee7d072c6ec4511102a125592e5ad3534f4194/src/controller/avdeccControllerImplHandlers.cpp#L157-L164

Additionally it is currently expected that if any dynamic_info contains an error status, the GET_DYNAMIC_INFO also has an error status (in processGetDynamicInfoFailureStatus()). While this is (AFAICT) under-specified in 1722.1, I don't think this is a sensible assumption. I would expect that any successfully processed GET_DYNAMIC_INFO command would have a SUCCESS response status.

christophe-calmejane commented 11 months ago

Nice catch, bad copy-paste (I know I shouldn't have finish this code during a plugfest). Should be a continue of course.

Regarding the specification, the main status code is only related to the command itself, not at all to any of the subcommands (eg. an error in a subcommand has no effect on the main status code). On the controller side, I chose to reject the whole command if there is any error and fallback to the individual enumeration commands for ease of implementation (may be improved at a later time). I will add a comment explaining this.

Edit: Well actually after rethinking this, the current code makes sense as it is by choice that the whole command is rejected at the first encountered error, even though we already processed some commands, it has no consequences to get the same info twice (for the model, not for performance). That's the reason why we break from the loop for the time being. I'll still add a note so I don't forget to switch from a break to a continue when I'll improve this code!

Florob commented 11 months ago

Regarding the specification, the main status code is only related to the command itself, not at all to any of the subcommands (eg. an error in a subcommand has no effect on the main status code).

That does however mean that it should be expected that the main status code can be SUCCESS, while one of the subcommands errors. Currently there is an assert that checks this is not the case, bringing down the application in debug builds.

christophe-calmejane commented 11 months ago

I don't see any assert related to what you describe. Can you provide the line and assert message?

Florob commented 11 months ago

Sure. I'm speaking of the assert in https://github.com/L-Acoustics/avdecc/blob/55ee7d072c6ec4511102a125592e5ad3534f4194/src/controller/avdeccControllerImpl.cpp#L5104-L5106

Which fires in the context of https://github.com/L-Acoustics/avdecc/blob/55ee7d072c6ec4511102a125592e5ad3534f4194/src/controller/avdeccControllerImplHandlers.cpp#L555-L563

where gotError can be true, while updatedStatus remains SUCCESS. Now that I look at the naming, maybe the intend was to actually update updatedStatus with the subcommand's status code?

christophe-calmejane commented 11 months ago

Oh right, I see. I couldn't give that code much test and wanted it merged so other manufacturer can start implementing GET_DYNAMIC_INFO so it's great to see another one. I'll try to update things a bit soon. Thx