Introduce variation of DeviceHandlerBase which uses two-step communication #166

New Issue

muellerr · 2020-08-17T18:30:05+02:00

muellerr commented

2020-08-17 18:30:05 +02:00

DHB was developed based on the RMAP protocol which uses 4 step communication.

The drivers we are using (SPI/I2C/UART) generally involve 2-step (I2C / UART) or 1-step (SPI, full-duplex) communication, using a DMA engine.

What do you think about introducing a simplified version of the DHB which just uses two-step-communication? This would also require a simpler version of DeviceCommunicationIF.

Maybe the communication steps can be packaged in a separate helper class (CommunicationHelper ?). The DHB code could mostly stay the same by transferring everything communication related to the helper.

DHB then identifies whether the communication helper is a 4-step or 2-step variation and calls a respective member function of the helper.

DHB was developed based on the RMAP protocol which uses 4 step communication. The drivers we are using (SPI/I2C/UART) generally involve 2-step (I2C / UART) or 1-step (SPI, full-duplex) communication, using a DMA engine. What do you think about introducing a simplified version of the DHB which just uses two-step-communication? This would also require a simpler version of DeviceCommunicationIF. Maybe the communication steps can be packaged in a separate helper class (`CommunicationHelper` ?). The DHB code could mostly stay the same by transferring everything communication related to the helper. DHB then identifies whether the communication helper is a 4-step or 2-step variation and calls a respective member function of the helper.

muellerr added the

feature

label 2020-08-17 18:30:05 +02:00

muellerr changed title from ~~Introduce variation of DHB which uses two-step communication~~ to Introduce variation of DeviceHandlerBase which uses two-step communication

2020-08-17 18:30:14 +02:00

muellerr commented

2022-08-31 11:34:14 +02:00

After hours of debugging of a payload handler yesterday, some weaknesses of the current DHB command handling become apparent:

Inflexibility in the face of more complex communication schemes. For the PLOC MPSOC, multiple replies were received for each command and some commands also take a longer time. The solar cell experiment has some commands which are simple, but there are also commands which take 2-3 minutes.
The tendency to pull users into DHB internals. This is usually a very frustrating experience. Ideally, users are exposed to these internals as rarely as possible. Practically, developers often have to step through a lot of DHB code as soon as something is done which does not fit 100 % into the DHB communication scheme
DHB command and reply handling is overkill for simple interfaces like I2C and SPI
DHB command and reply handling is too inflexible for interfaces like UART, which often times require custom solutions, especially when it comes to reply handling, which might or might not have data link layers.
The DeviceComIF being generic is a luxury which is often times not required for various reasons. I have no issue with something like the PLOC handling being custom code completely tailored towards the mission. Even if it is required, one would be able to use a custom abstraction passed externally, instead of being builtin.

i think the DHB has a lot of useful featutes which are necassary for it to fit into other FSFW concepts, but the command/reply handling is something where a little bit of rework might be worth the effort.

This was mostly related to COM handling. I think it would be a very good idea and very much possible to provide a more flexible and user driven way to perform COM handling. It should be possible for users (especially beginners, who want to write working code without speding weeks understanding DHB and its whole backbone) to write device handlers for simple sensors. An interface which would allow something could look like this for a SPI sensor

DeviceHandlerBase.h

virtual ReturnvalueT handleDeviceCommunication(SomeHelperWrapper wrapper) = 0;

MySpiSensor.h

// ctor
MySpiSensor(/* other params,  DHB params ,object ID, etc. */, ConcreteSpiComIF* comIF)
   : DeviceHandlerBase(...), spiComIF(comIF)

ReturnvalueT handleDeviceCommunication(SomeHelperWrapper wrapper) override;

MySpiSensor.cpp

ReturnvalueT handleDeviceCommunication(SomeHelperWrapper wrapper {
	DeviceCommandId_t id = wrapper.getPendingId();
    if (id == Ids::REQUEST_TEMPS) {
        // command and reply handling in one
    	spiComIF->transfer(...);
    }
    wrapper.confirmCommandDone(id);
    wrapper.wiretappingWrapper(replyBuf);
    wrapper.fdirWrapper(...);
}

For generic communication schemes with 2 step or 4 step communication (no custom handling needed), some generic abstract class could be provided, and the user can use the implementation of this abstract class to the handler.

After hours of debugging of a payload handler yesterday, some weaknesses of the current DHB command handling become apparent: - Inflexibility in the face of more complex communication schemes. For the PLOC MPSOC, multiple replies were received for each command and some commands also take a longer time. The solar cell experiment has some commands which are simple, but there are also commands which take 2-3 minutes. - The tendency to pull users into DHB internals. This is usually a very frustrating experience. Ideally, users are exposed to these internals as rarely as possible. Practically, developers often have to step through a lot of DHB code as soon as something is done which does not fit 100 % into the DHB communication scheme - DHB command and reply handling is overkill for simple interfaces like I2C and SPI - DHB command and reply handling is too inflexible for interfaces like UART, which often times require custom solutions, especially when it comes to reply handling, which might or might not have data link layers. - The DeviceComIF being generic is a luxury which is often times not required for various reasons. I have no issue with something like the PLOC handling being custom code completely tailored towards the mission. Even if it is required, one would be able to use a custom abstraction passed externally, instead of being builtin. i think the DHB has a lot of useful featutes which are necassary for it to fit into other FSFW concepts, but the command/reply handling is something where a little bit of rework might be worth the effort. This was mostly related to COM handling. I think it would be a very good idea and very much possible to provide a more flexible and user driven way to perform COM handling. It should be possible for users (especially beginners, who want to write working code without speding weeks understanding DHB and its whole backbone) to write device handlers for simple sensors. An interface which would allow something could look like this for a SPI sensor DeviceHandlerBase.h ```cpp virtual ReturnvalueT handleDeviceCommunication(SomeHelperWrapper wrapper) = 0; ``` MySpiSensor.h ```cpp // ctor MySpiSensor(/* other params, DHB params ,object ID, etc. */, ConcreteSpiComIF* comIF) : DeviceHandlerBase(...), spiComIF(comIF) ReturnvalueT handleDeviceCommunication(SomeHelperWrapper wrapper) override; ``` MySpiSensor.cpp ```cpp ReturnvalueT handleDeviceCommunication(SomeHelperWrapper wrapper { DeviceCommandId_t id = wrapper.getPendingId(); if (id == Ids::REQUEST_TEMPS) { // command and reply handling in one spiComIF->transfer(...); } wrapper.confirmCommandDone(id); wrapper.wiretappingWrapper(replyBuf); wrapper.fdirWrapper(...); } ``` For generic communication schemes with 2 step or 4 step communication (no custom handling needed), some generic abstract class could be provided, and the user can use the implementation of this abstract class to the handler.

mohr commented

2022-08-31 13:15:43 +02:00

At this point I would rather suggest writing a new BaseClass.

DHB has its use and in my view its justification but as you have shown more than once is not suitable for all system designs.

As DHB has always been seen as too complex I do not think adding functionality will make it much easier to use.

I think writing a new Base class from scratch with the concepts learned with EIVE in mind will produce a much nicer interface for users. This BaseClass can then be using a two step process (with POSIX write and read class in mind) and skip the generic DeviceComIF which in a way was written as a complement to the POSIX API which was not available for Flying Laptop. As such it seems quite consistant to replace the DeviceComIF with a read/write interface which usually is backed by a driver implementation in the operating system. This would also replace the scanForReply() layer in DHB which is again a complement to OS level drivers for a framing layer.

By this, hopefully a lot of the complexity of DHB can be removed while keeping DHB for cases where the complexity is needed.

At this point I would rather suggest writing a new BaseClass. DHB has its use and in my view its justification but as you have shown more than once is not suitable for all system designs. As DHB has always been seen as too complex I do not think adding functionality will make it much easier to use. I think writing a new Base class from scratch with the concepts learned with EIVE in mind will produce a much nicer interface for users. This BaseClass can then be using a two step process (with POSIX write and read class in mind) and skip the generic `DeviceComIF` which in a way was written as a complement to the POSIX API which was not available for Flying Laptop. As such it seems quite consistant to replace the `DeviceComIF` with a read/write interface which usually is backed by a driver implementation in the operating system. This would also replace the `scanForReply()` layer in DHB which is again a complement to OS level drivers for a framing layer. By this, hopefully a lot of the complexity of DHB can be removed while keeping DHB for cases where the complexity is needed.

muellerr commented

2022-08-31 13:30:59 +02:00

A POSIX write and read interface also sounds nice. Would those be abstract functions?

On bare metal systems, a POSIX API might not be present for I2C/SPI/UART, but most of the times something similar is available.

A POSIX `write` and `read` interface also sounds nice. Would those be abstract functions? On bare metal systems, a POSIX API might not be present for I2C/SPI/UART, but most of the times something similar is available.

mohr commented

2022-08-31 16:40:56 +02:00

I meant that the Base class is designed to be coupled to an API which behaves like POSIX read/write.
But I guess there needs something like CommunicationIF, just extremely lightweight, where one can plug either POSIX, if available, or some bare metal driver which then needs to provide a suitable interface. That is a definition for the interface between the handler and the driver. Whether that driver is coupled with a static object as in DHB or as a member object would be to be defined.

I meant that the Base class is designed to be coupled to an API which behaves like POSIX read/write. But I guess there needs something like CommunicationIF, just extremely lightweight, where one can plug either POSIX, if available, or some bare metal driver which then needs to provide a suitable interface. That is a definition for the interface between the handler and the driver. Whether that driver is coupled with a static object as in DHB or as a member object would be to be defined.

muellerr referenced this issue from eive/fsfw

2023-11-09 10:38:13 +01:00

New simpler DHB #161

Sign in to join this conversation.