DHB coupling #99

New Issue

muellerr · 2020-06-06T12:22:26+02:00

muellerr commented

2020-06-06 12:22:26 +02:00

I have architectural suggestions for DHB, which would improve its maintainability, readability and ease of use (especially for newer projects).

DHB is coupled to other components:

PowerSwitcherIF
ConfirmsFailuresIF
Raw Data Receiver
HasHealthIF
DeviceComIF
CookieF

It would be better if one could just instantiate a device handlers without worrying about all these other objects (there is a default FDIR for example). I started development with a DummyDeviceHandler. I did not need a the health table, power switchers, confirm failures and a raw data receiver but I just implemented them eventually.

Now I am trying to get a device handler running on linux to be able to test local pools of 2 OSes and I have the same exact problem: Components are missing (coupling!). In my opinion this is okay for objects like DeviceComIF and CookieIF which are just passed in the CTOR, but for objects like the Health Helper I don't even get a proper warning, I have to step through initialize() to find out it was a missing health table.

I suggest that the first 4 components are decoupled from DHB as a first step. This is easier for Health Table, which are only accessed via the helper interface ( with the little architectural question whether their active state is checked in the helper class or inside the device handler. I'd say that it would be better if this is handled in the DHB) The first 2 maybe should be packaged inside a helper(e.g. PowerHelper?) as well as the third one (e.g. DeviceHandlerHelper, wich handles raw traffic/wiretapping?).

Doing that has other benefits: Unit testing the components of DHB becomes easier/possible. For components like the helpers, this is already possible using mocks und their public interface, but if the logic is spread over a lot of functions which are private as they should be, unit testing them (and thus making refactoring more safe and easy) becomes difficult or impossible.

Performing all of those steps will propably also decrease the LOC of the main module and increase its readability. Of course, the logic will be spread over multiple files, but the logic of a certain domain (like power handling) will be packaged neatly inside a helper module. This might also decrease the number of instance variables (currently 20+ I think) or at the very least transform them into private variables which are passed to the helper, if the helper has to set a state of its owner.

I have architectural suggestions for DHB, which would improve its maintainability, readability and ease of use (especially for newer projects). DHB is coupled to other components: - PowerSwitcherIF - ConfirmsFailuresIF - Raw Data Receiver - HasHealthIF - DeviceComIF - CookieF It would be better if one could just instantiate a device handlers without worrying about all these other objects (there is a default FDIR for example). I started development with a DummyDeviceHandler. I did not need a the health table, power switchers, confirm failures and a raw data receiver but I just implemented them eventually. Now I am trying to get a device handler running on linux to be able to test local pools of 2 OSes and I have the same exact problem: Components are missing (coupling!). In my opinion this is okay for objects like DeviceComIF and CookieIF which are just passed in the CTOR, but for objects like the Health Helper I don't even get a proper warning, I have to step through initialize() to find out it was a missing health table. I suggest that the first 4 components are decoupled from DHB as a first step. This is easier for Health Table, which are only accessed via the helper interface ( with the little architectural question whether their active state is checked in the helper class or inside the device handler. I'd say that it would be better if this is handled in the DHB) The first 2 maybe should be packaged inside a helper(e.g. PowerHelper?) as well as the third one (e.g. DeviceHandlerHelper, wich handles raw traffic/wiretapping?). Doing that has other benefits: Unit testing the components of DHB becomes easier/possible. For components like the helpers, this is already possible using mocks und their public interface, but if the logic is spread over a lot of functions which are private as they should be, unit testing them (and thus making refactoring more safe and easy) becomes difficult or impossible. Performing all of those steps will propably also decrease the LOC of the main module and increase its readability. Of course, the logic will be spread over multiple files, but the logic of a certain domain (like power handling) will be packaged neatly inside a helper module. This might also decrease the number of instance variables (currently 20+ I think) or at the very least transform them into private variables which are passed to the helper, if the helper has to set a state of its owner.

muellerr added the

feature

question

labels 2020-06-06 12:22:26 +02:00

muellerr commented

2020-07-09 16:33:18 +02:00

UPDATE:

What do you think about setting optional ctor arguments in separate setter functions?
Something like this:

	/**
	 * The constructor passes the objectId to the SystemObject().
	 *
	 * @param setObjectId the ObjectId to pass to the SystemObject() Constructor
	 * @param maxDeviceReplyLen the length the RMAP getRead call will be sent with
	 * @param setDeviceSwitch the switch the device is connected to,
	 * for devices using two switches, overwrite getSwitches()
	 * @param deviceCommuncation Communcation Interface object which is used
	 * to implement communication functions
	 * @param thermalStatePoolId
	 * @param thermalRequestPoolId
	 * @param fdirInstance
	 * @param cmdQueueSize
	 */
	DeviceHandlerBase(object_id_t setObjectId, object_id_t deviceCommunication,
			CookieIF * comCookie, FailureIsolationBase* fdirInstance = nullptr,
			size_t cmdQueueSize = 20);

	void setDeviceSwitch(uint8_t deviceSwitch);
	void setHkDestination(object_id_t hkDestination);
	void setThermalStateRequestPoolIds(uint32_t thermalStatePoolId,
			uint32_t thermalRequestPoolId);

If these objects are not set, their default values will put the DHB in a state which does not use the functionalities.
If HK destination is not set, the static framework ID defaultHkDestination will be set, which can be specified in the factory. That way, it is possible to set the HK destination of all device handlers to a default service (for periodic handling), and set the destination for individual device handlers to another HK destination, if that is needed.

Also, excerpt of the changed initialize() function to make raw receiver and PowerSwitch optional

if(rawDataReceiverId != objects::NO_OBJECT) {
	AcceptsDeviceResponsesIF *rawReceiver = objectManager->get<
			AcceptsDeviceResponsesIF>(rawDataReceiverId);

	if (rawReceiver == nullptr) {
		sif::error << "DeviceHandlerBase::initialize: Raw receiver object "
				"ID set but no valid object found." << std::endl;
		sif::error << "Make sure the raw receiver object is set up properly"
				" and implements AcceptsDeviceResponsesIF" << std::endl;
		return ObjectManagerIF::CHILD_INIT_FAILED;
	}
	defaultRawReceiver = rawReceiver->getDeviceQueue();
}

if(powerSwitcherId != objects::NO_OBJECT) {
	powerSwitcher = objectManager->get<PowerSwitchIF>(powerSwitcherId);
	if (powerSwitcher == nullptr) {
		sif::error << "DeviceHandlerBase::initialize: Power switcher "
				<< "object ID set but no valid object found." << std::endl;
		sif::error << "Make sure the raw receiver object is set up properly"
				<< " and implements PowerSwitchIF" << std::endl;
		return ObjectManagerIF::CHILD_INIT_FAILED;
	}
}

of course, there are more checks inside the code now which ensure that power switcher won't be called if it is not set.

UPDATE: What do you think about setting optional ctor arguments in separate setter functions? Something like this: ```cpp /** * The constructor passes the objectId to the SystemObject(). * * @param setObjectId the ObjectId to pass to the SystemObject() Constructor * @param maxDeviceReplyLen the length the RMAP getRead call will be sent with * @param setDeviceSwitch the switch the device is connected to, * for devices using two switches, overwrite getSwitches() * @param deviceCommuncation Communcation Interface object which is used * to implement communication functions * @param thermalStatePoolId * @param thermalRequestPoolId * @param fdirInstance * @param cmdQueueSize */ DeviceHandlerBase(object_id_t setObjectId, object_id_t deviceCommunication, CookieIF * comCookie, FailureIsolationBase* fdirInstance = nullptr, size_t cmdQueueSize = 20); void setDeviceSwitch(uint8_t deviceSwitch); void setHkDestination(object_id_t hkDestination); void setThermalStateRequestPoolIds(uint32_t thermalStatePoolId, uint32_t thermalRequestPoolId); ``` If these objects are not set, their default values will put the DHB in a state which does not use the functionalities. If HK destination is not set, the static framework ID defaultHkDestination will be set, which can be specified in the factory. That way, it is possible to set the HK destination of all device handlers to a default service (for periodic handling), and set the destination for individual device handlers to another HK destination, if that is needed. Also, excerpt of the changed initialize() function to make raw receiver and PowerSwitch optional ```cpp if(rawDataReceiverId != objects::NO_OBJECT) { AcceptsDeviceResponsesIF *rawReceiver = objectManager->get< AcceptsDeviceResponsesIF>(rawDataReceiverId); if (rawReceiver == nullptr) { sif::error << "DeviceHandlerBase::initialize: Raw receiver object " "ID set but no valid object found." << std::endl; sif::error << "Make sure the raw receiver object is set up properly" " and implements AcceptsDeviceResponsesIF" << std::endl; return ObjectManagerIF::CHILD_INIT_FAILED; } defaultRawReceiver = rawReceiver->getDeviceQueue(); } if(powerSwitcherId != objects::NO_OBJECT) { powerSwitcher = objectManager->get<PowerSwitchIF>(powerSwitcherId); if (powerSwitcher == nullptr) { sif::error << "DeviceHandlerBase::initialize: Power switcher " << "object ID set but no valid object found." << std::endl; sif::error << "Make sure the raw receiver object is set up properly" << " and implements PowerSwitchIF" << std::endl; return ObjectManagerIF::CHILD_INIT_FAILED; } } ``` of course, there are more checks inside the code now which ensure that power switcher won't be called if it is not set.

gaisser commented

2020-10-27 14:49:51 +01:00

DHB is not hard coupled with the following anymore:

PowerSwitcherIF
ConfirmsFailuresIF
Raw Data Receiver

If any of those are not intiated a error is printed to the sif:

HealthTable (required through HealthHelper)
DeviceComIF (A ObjectId to a valid SystemObject is required)
CookieIF (A pointer to valid CookieIF is required in the constructor of DHB. Nullptr checks are implemented)

I consider this solved with the current state.

DHB is not hard coupled with the following anymore: * PowerSwitcherIF * ConfirmsFailuresIF * Raw Data Receiver If any of those are not intiated a error is printed to the sif: HealthTable (required through HealthHelper) DeviceComIF (A ObjectId to a valid SystemObject is required) CookieIF (A pointer to valid CookieIF is required in the constructor of DHB. Nullptr checks are implemented) I consider this solved with the current state.

gaisser closed this issue

2020-10-27 14:51:55 +01:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: fsfw/fsfw#99