avdecc
avdecc copied to clipboard
L-Acoustics
dynamic_info responses in GET_DYNAMIC_INFO are not handled separately
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.
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!
Regarding the specification, the
mainstatus 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 themainstatus 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.
I don't see any assert related to what you describe. Can you provide the line and assert message?
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?
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