DeviceHandlerBase: All Refactoring #44

muellerr · 2020-04-19T20:34:37+02:00

muellerr commented

2020-04-19 20:34:37 +02:00

This fixes #19 .
This fixes #41 .

However, the changes discussed on 01.04 includes more than just DeviceCommunicationIF, so maybe a new issue would be good to discuss all the changes I coded.

Repitition of points from #19:

Replace the open call and allocate the cookies in the factory and hand the pointer to the DeviceCommunicationIF
Add a request length for requestReceiveMessage
Find better names for all functions
Document the interaction patterns

Unfortunately, there are some several API changes after looking at this topic and implementing the SPI and I2C communication interfaces for SOURCE.

I also found out that an separate initialization function called in the DeviceHandlerBase (that was open(), I renamed it to initializeInterface()) is still needed because I need to initialize data structures in the communication interface implementation with information stored inside the cookie (e.g. for a map of addresses to Com Status information).

Here is a list of the various changes. I have several branches which implemented subsets of these changes, but I think it would be best to gather them into one pull request as they are connected to each other.

Some clarification required:
What is the meaning of the returnvalues located in DHB, and the ones in DeviceHandlerIF? Did I understand correctly that the returnvalues in DHB alter the behaviour of DHB while the returnvalues in DeviceHandlerIF are just error codes? If it is supposed to be like that, I suggest moving the rest of the error codes (2-3 left I think) to DeviceHandlerIF too.

UPDATE: Actually, the returnvalues of DHB are "private" to DHB, so its okay like that.

`Cookie` or `CookieIF`

Renamed to CookieIF (see #41 ), functionality stays the same.
Documentation added. There is a separate pull request #42 but this change is also included in #44.

How to integrate change: Rename Cookie to CookieIF

`DeviceHandlerBase`

No diff for header file , needs to be diffed from command line.

Constructor/API changes

Does not take and cache ioBoardAddress and maxReplyLength anymore. It is assumed that these variables are passed to the Cookie in the factory construction
Takes a CookieIF pointer, which is cached.

Functional changes

switchCookieChannel function commented out for now
DeviceReplyMap has a new entry called replyLen.
Its value can be set in insertInCommandReplyMap on device handler initialization (or updateCommandReplyMap). It is used to pass the correct replyLength in the requestReceiveMessage() call to the communication interface.

Documentation / Formatting / Code Style etc.

Functions in header resorted in order of importance for someone implementing child handlers. The abstract functions are closer at the top now as these must be implemented. I grouped together insertInCommandAndReplyMap and its helper functions.
getSwitches and getTransitionDelay closer to the top too.
additional documentation added or corrected for some functions. There was an explanation in DeviceHandlerBase.h I did not understand (line 350).
reduced size of constructor by moving 0 or nullptr initialization (NULL replaced by nullptr) into the header.
I reduced the number of includes and sorted them a bit.

Enhancement

new returnvalue IGNORE_FULL_PACKET for scanForReply() function. If this is returned, DHB will stop parsing and ignore the reply
performOperationHook() : A virtual hook function which will be called once per performOperation. Default implementation empty.
debugInterface() a debug interface. If I put a printout into the DHB, it will be printed for each child handler. If I implement the debugInterface() in the respective child handler (default implementation is empty), I can limit printouts to the target child handler to debug. This can be also useful for breakpoints. I can just set the breakpoint in the child handler debugInterface() to only get stepping for a target child handler.

How to integrate change

Cookies are now instantiated in the factory instead of the communication interface
CommunicationIF specific configuration can be performed in the initializeInterface() function call
The DHB constructor arguments maxReplyLen and ioBoardAddress were replaced by CookieIF, which can also contain these values, depending on cookie configuration, so all calls to DHB have to be adapted.

`DeviceCommunicationIF`

getParameter()/setParameter() and setAddress()/getAddress() removed for now
reOpen() removed
open() replaced by initializeInterface(CookieIF* cookie)
sendMessage() data pointer is constant
requestReceiveMessage() also supplies a replyLength in addition to the cookie. If a sequential communication system is used, the correct replyLength should be taken from the reply map and will be passed to the communication interface. Otherwise, 0 is passed and the communication interface can device to read everything or nothing.
I added a new returnvalue NO_REPLY_RECEIVED, which can be used in receiveRequestedReply() to explicitely state that no reply was received.

How to integrate change

The removed functions don't have to be implemented anymore.
open() calls need to be renamed to initializeInterface(). A constructed cookie is passedf to the ComIF, which can then be modified or used to initialize comIF specific parameters or data structures.
Input arguments of some other abstract functions have changed.
Cookie initialization moved from Child com interface to factory.

`DeviceHandlerIF`

I think there was some issue with the indentation.
I improved the readability of this file by writing the explanation
of the modes above the mode definitions and keeping to the 80 column widht limit
(hope that did not break code parsers).
I also renamed the RmapAction to CommunicationAction (more generic name).

Label: API Change, Documentation, Enhancement

This fixes #19 . This fixes #41 . However, the changes discussed on 01.04 includes more than just DeviceCommunicationIF, so maybe a new issue would be good to discuss all the changes I coded. Repitition of points from #19: - Replace the open call and allocate the cookies in the factory and hand the pointer to the DeviceCommunicationIF - Add a request length for requestReceiveMessage - Find better names for all functions - Document the interaction patterns Unfortunately, there are some several API changes after looking at this topic and implementing the SPI and I2C communication interfaces for SOURCE. I also found out that an separate initialization function called in the DeviceHandlerBase (that was `open()`, I renamed it to `initializeInterface()`) is still needed because I need to initialize data structures in the communication interface implementation with information stored inside the cookie (e.g. for a map of addresses to Com Status information). Here is a list of the various changes. I have several branches which implemented subsets of these changes, but I think it would be best to gather them into one pull request as they are connected to each other. Some clarification required: What is the meaning of the returnvalues located in DHB, and the ones in DeviceHandlerIF? Did I understand correctly that the returnvalues in DHB alter the behaviour of DHB while the returnvalues in DeviceHandlerIF are just error codes? If it is supposed to be like that, I suggest moving the rest of the error codes (2-3 left I think) to DeviceHandlerIF too. UPDATE: Actually, the returnvalues of DHB are "private" to DHB, so its okay like that. ## `Cookie` or `CookieIF` Renamed to CookieIF (see #41 ), functionality stays the same. Documentation added. There is a separate pull request #42 but this change is also included in #44. How to integrate change: Rename Cookie to CookieIF ## `DeviceHandlerBase` No diff for header file , needs to be diffed from command line. #### Constructor/API changes 1. Does not take and cache ioBoardAddress and maxReplyLength anymore. It is assumed that these variables are passed to the Cookie in the factory construction 2. Takes a CookieIF pointer, which is cached. #### Functional changes 1. `switchCookieChannel` function commented out for now 2. DeviceReplyMap has a new entry called replyLen. Its value can be set in insertInCommandReplyMap on device handler initialization (or updateCommandReplyMap). It is used to pass the correct replyLength in the `requestReceiveMessage()` call to the communication interface. #### Documentation / Formatting / Code Style etc. 1. Functions in header resorted in order of importance for someone implementing child handlers. The abstract functions are closer at the top now as these must be implemented. I grouped together insertInCommandAndReplyMap and its helper functions. 2. getSwitches and getTransitionDelay closer to the top too. 3. additional documentation added or corrected for some functions. There was an explanation in `DeviceHandlerBase.h` I did not understand (line 350). 4. reduced size of constructor by moving 0 or nullptr initialization (NULL replaced by nullptr) into the header. 5. I reduced the number of includes and sorted them a bit. #### Enhancement 1. new returnvalue IGNORE_FULL_PACKET for scanForReply() function. If this is returned, DHB will stop parsing and ignore the reply 2. performOperationHook() : A virtual hook function which will be called once per performOperation. Default implementation empty. 3. debugInterface() a debug interface. If I put a printout into the DHB, it will be printed for each child handler. If I implement the debugInterface() in the respective child handler (default implementation is empty), I can limit printouts to the target child handler to debug. This can be also useful for breakpoints. I can just set the breakpoint in the child handler debugInterface() to only get stepping for a target child handler. #### How to integrate change 1. Cookies are now instantiated in the factory instead of the communication interface 2. CommunicationIF specific configuration can be performed in the `initializeInterface()` function call 3. The DHB constructor arguments maxReplyLen and ioBoardAddress were replaced by CookieIF, which can also contain these values, depending on cookie configuration, so all calls to DHB have to be adapted. ## `DeviceCommunicationIF` 1. `getParameter()`/`setParameter()` and `setAddress()`/`getAddress()` removed for now 2. `reOpen()` removed 3. `open()` replaced by `initializeInterface(CookieIF* cookie)` 4. `sendMessage()` data pointer is constant 5. `requestReceiveMessage()` also supplies a replyLength in addition to the cookie. If a sequential communication system is used, the correct replyLength should be taken from the reply map and will be passed to the communication interface. Otherwise, 0 is passed and the communication interface can device to read everything or nothing. 6. I added a new returnvalue `NO_REPLY_RECEIVED`, which can be used in `receiveRequestedReply()` to explicitely state that no reply was received. #### How to integrate change 1. The removed functions don't have to be implemented anymore. 2. open() calls need to be renamed to initializeInterface(). A constructed cookie is passedf to the ComIF, which can then be modified or used to initialize comIF specific parameters or data structures. 3. Input arguments of some other abstract functions have changed. 4. Cookie initialization moved from Child com interface to factory. ## `DeviceHandlerIF` I think there was some issue with the indentation. I improved the readability of this file by writing the explanation of the modes above the mode definitions and keeping to the 80 column widht limit (hope that did not break code parsers). I also renamed the RmapAction to CommunicationAction (more generic name). Label: API Change, Documentation, Enhancement

~~muellerr referenced this pull request 2020-04-19 20:47:40 +02:00~~

DeviceHandlerBase - Refactoring - Discussion #45

~~muellerr referenced this pull request 2020-04-19 20:54:46 +02:00~~

DeviceHandlerBase - Refactoring - Discussion #45

mohr added the

feature

API Change

labels 2020-04-20 14:27:36 +02:00

mohr self-assigned this 2020-04-20 14:27:45 +02:00

mohr referenced this pull request

2020-04-20 14:52:08 +02:00

DeviceHandlerBase - Refactoring - Discussion #45