dynamic_info responses in GET_DYNAMIC_INFO are not handled separately

[Florob]

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:

avdecc/src/controller/avdeccControllerImplHandlers.cpp

Lines 157 to 164 in 55ee7d0

	for (auto const& [st, commandType, arguments] : resultParameters)
	{
	gotError |= !st;
	if (gotError)
	{
	LOG_CONTROLLER_DEBUG(entityID, "GetDynamicInfo failed for commandType={} with status={}", static_cast<std::string>(commandType), entity::ControllerEntity::statusToString(st));
	break;
	}

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]

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]

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]

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

[Florob]

Sure. I'm speaking of the assert in

avdecc/src/controller/avdeccControllerImpl.cpp

Lines 5104 to 5106 in 55ee7d0

	bool ControllerImpl::processEmptyGetDynamicInfoFailureStatus(entity::ControllerEntity::AemCommandStatus const status, ControlledEntityImpl* const entity) noexcept
	{
	AVDECC_ASSERT(!status, "Should not call this method with a SUCCESS status");

Which fires in the context of

avdecc/src/controller/avdeccControllerImplHandlers.cpp

Lines 555 to 563 in 55ee7d0

	if (gotError)
	{
	if (!processGetDynamicInfoFailureStatus(updatedStatus, &entity, sentParameters, packetID, step))
	{
	controlledEntity->setGetFatalEnumerationError();
	notifyObserversMethod<Controller::Observer>(&Controller::Observer::onEntityQueryError, this, &entity, QueryCommandError::GetDynamicInfo);
	return;
	}
	}

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]

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