Skip to main content


Eclipse Community Forums
Forum Search:

Search      Help    Register    Login    Home
Home » Eclipse Projects » 4DIAC - Framework for Distributed Industrial Automation and Control » Communication return codes enhancement proposal(A proposal for a new return code scheme that improves the current one)
icon3.gif  Communication return codes enhancement proposal [message #1758064] Thu, 23 March 2017 08:45 Go to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
The code scheme being returned by the communication architecture (shown in the image) is using some return codes for multiple purposes that do not allow to differenciate them. For example, SERVERS are getting an e_InitTerminated both when they are asked to be shutdown by a INIT- and when a client gets disconnected.

comtypes.h (lines 21-53)

Right now the bits of the return code are:

  1. Error code
  2. Error code
  3. Error code
  4. Error code
  5. Flag for INIT return codes
  6. Flag for non-INIT return codes
  7. Flag for unsuccessful operation
  8. Unused

8 error codes require 4 bits, then 2 flags are being used for opposite states that would only require one (using two makes checking them a bit easier as you can use two different masks instead of just one and negating the result) and a flag for unsuccessful responses which that can also be matched negating e_Ok as every other code is an error.

These 3 enums and their members appear 957 times (702 e_ProcessDataOk is the most repeated one) in the whole project (including tests) unless there is some variable name overlapping, so before changing all of them and submitting a merge request I would love to hear feedback.

My proposed scheme would be the following:

  1. Error code
  2. Error code
  3. Error code
  4. Error code
  5. Reserved
  6. Reserved
  7. Event code
  8. Event code

The reserved bits are available if some additional error or event codes want to be introduced. The highest 2 bits (event code) determine which event the return code belongs to:

  • 0 (00): INITO
  • 1 (01): CNF
  • 2 (10): IND
  • 3 (11): -

Despite the current error codes can be mapped to just 3 bits by overlapping codes that can't happen under the same event codes such as Invalid ID and Invalid Object, I've decided against this approach as checking if an error code is of some kind may give false positives or require event matching too.

  • 0 (0000): Nothing (INITO, CNF, IND)
  • 1 (0001): Ok (INITO, CNF, IND)
  • 2 (0010): Invalid ID (INITO)
  • 3 (0011): Terminated (INITO)
  • 4 (0100): Invalid Object (CNF, IND)
  • 5 (0101): Data type error (CNF, IND)
  • 6 (0110): Inhibited (CNF)
  • 7 (0111): No socket (CNF)
  • 8 (1000): Overflow (CNF, IND)
  • 9 (1001): Underflow (CNF, IND)
  • 10-15 (1010-1111): -


This scheme makes possible to differenciate both the code and the event separately. It has been designed with scalability in mind in case some new error or event codes want to be introduced. For this purpose a constant has been set to determine how many bits are used for event codes even if this number will probably not increase. Operators & overloading allow for easy event or error code checking and I'm currently implementing operators for code assigning (|= and &=, the first will only assign the new code if the previous is e_Nothing or e_Ok while the second one will always update the error code), and other common operations like conversion between the 3 enum types and so on. I implemented a demo of how this works in repl.it.

[Updated on: Thu, 30 March 2017 07:09]

Report message to a moderator

Re: Communication return codes enhancement proposal [message #1758082 is a reply to message #1758064] Thu, 23 March 2017 11:09 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

I think it is a great idea to make it more exlpcit what return values are allowed. I only don't think that ok and nothing need to be typed for the event. I think it makes the code better readable if these normal operation cases are handled uniformly.

What your proposal triggered in my mind: Would it make sense to have three different enumrations:

  1. open/close connection,
  2. recv
  3. send

Re: Communication return codes enhancement proposal [message #1758104 is a reply to message #1758082] Thu, 23 March 2017 14:38 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Alois Zoitl wrote on Thu, 23 March 2017 11:09
I think it is a great idea to make it more exlpcit what return values are allowed. I only don't think that ok and nothing need to be typed for the event. I think it makes the code better readable if these normal operation cases are handled uniformly.


With my scheme this is doable, you can take a look at the decompose() function. These two conditions will be true for any event:
if(resp & e_Nothing) {...}
if(resp & e_Ok) {...}


Alois Zoitl wrote on Thu, 23 March 2017 11:09
What your proposal triggered in my mind: Would it make sense to have three different enumrations:

  1. open/close connection,
  2. recv
  3. send



Separating the enums into 3 different ones would also be a nice idea.

Either way, the following variable appearances (957 in total) should be changed:

  1. scg_unInit (1)
  2. scg_unComNegative (2)
  3. e_Terminated (1)
  4. EComResponse (139)
  5. e_Nothing (30)
  6. e_InitOk (16)
  7. e_ProcessDataOk (702)
  8. e_InitInvalidId (21)
  9. e_InitTerminated (19)
  10. e_ProcessDataInvalidObject (1)
  11. e_ProcessDataDataTypeError (4)
  12. e_ProcessDataInhibited (1)
  13. e_ProcessDataNoSocket (6)
  14. e_ProcessDataSendFailed (8)
  15. e_ProcessDataRecvFaild (6)


From this 957 appearances, 671 are in 2 test files as can be seen in this image:
index.php/fa/28851/0/

[Updated on: Thu, 23 March 2017 15:12]

Report message to a moderator

Re: Communication return codes enhancement proposal [message #1758116 is a reply to message #1758104] Thu, 23 March 2017 16:52 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

Adrian Orive wrote on Thu, 23 March 2017 14:38
Alois Zoitl wrote on Thu, 23 March 2017 11:09
I think it is a great idea to make it more exlpcit what return values are allowed. I only don't think that ok and nothing need to be typed for the event. I think it makes the code better readable if these normal operation cases are handled uniformly.


With my scheme this is doable, you can take a look at the decompose() function. These two conditions will be true for any event:
if(resp & e_Nothing) {...}
if(resp & e_Ok) {...}



You are right I missed that.

Didn't expect taht we use the exisiting infrastrukture in so many places.

The reason why I suggested the three different enums is that it clearer defines for future layers how they should behave.
Re: Communication return codes enhancement proposal [message #1758171 is a reply to message #1758116] Fri, 24 March 2017 09:13 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Alois Zoitl wrote on Thu, 23 March 2017 16:52
Didn't expect taht we use the exisiting infrastrukture in so many places.


At least 70% are in test files, and they should be as easy as swapping them for their new version.

Alois Zoitl wrote on Thu, 23 March 2017 16:52
The reason why I suggested the three different enums is that it clearer defines for future layers how they should behave.


I agree. I have been further investigating this issue and I have come up with some additional conclussions/suggestions/thoughts:

  • INIT+: Every communication FB, independently of their type, handle positive INIT the same way with one exception that I have already mentioned and that I will talk about later.
    The ID is being parsed layer by layer, creating the appropiate layer and calling its virtual openConnection() method. During this process Invalid ID is already being returned if the ID can't be parsed, and the virtual openConnection() method should also be able to return Ok or Terminated in addition to Invalid ID. This virtual method is not required to call anything from the bottom layer as this is handled internally.
  • INIT-: Every communication FB, independently of their type, handle negative INIT the same way with one exception that I have already mentioned and that I will talk about later.
    If a layer is connected to the FB, it calls its virtual closeConnection() method, deletes the layer and returns Terminated. However, this virtual method is expected to call the bottom layers equivalent. A similar behaviour to INIT+ where this is not required would make the API more consistent in my opinion. Closing should probably start at the bottom layer instead, calling bottom's layer method before executing their own.
  • INIT+/- for Servers: This is the only exception to the above behaviour. Terminated return codes for servers are being silently ignored due to part of code that is intended to keep the listening open when a client shuts down. Moving to a three enum scheme (connection, send and receive, or what is the same, INIT, REQ/RSP and external events) will allow to differentiate this two return codes so that it doesn't silently ignores server closing.


TLDR:

  1. Swap to a three enums scheme: connection, send and receive
  2. Make closeConnection() behave similar to openConnection() in that it doesn't require to call bottom layers closeConnection(), preferably closing bottom layers before closing own

[Updated on: Mon, 27 March 2017 08:30]

Report message to a moderator

Re: Communication return codes enhancement proposal [message #1758176 is a reply to message #1758171] Fri, 24 March 2017 10:00 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Implementing my second suggestion would require to change 3 files plus the already implemented closeConnection() methods such as CRawDataComLayer::closeConnection() or CFBDKASN1ComLayer::closeConnection() (which would not be required anymore). I attached the 3 infrastructure files and the previously mentioned layers modified. I didn't commit it to git as I don't know if you like this approach. They all belong to /src/core/cominfra/.

Source: GIT's 1.8.x branch head, commit 79ceedbb4177fea33e8d33fb5fbfbb11adc67e0c by Monika Wenget @ 03/22/2017 15:28

[Updated on: Mon, 27 March 2017 08:30]

Report message to a moderator

Re: Communication return codes enhancement proposal [message #1758210 is a reply to message #1758176] Fri, 24 March 2017 17:00 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

wow that was fast. I try to read through the code over the weekend.

In response to your first post there are two thinks I wanted to say. I very much like the idea about having a generic close connection code in the base class of the layer. This allows it to have a private virtual closeConnection function in comlayer and even as you said for some layers wher we don't need to do anything remove one function in the specific layers.

I think there is a misunderstanding with INIT- of the server. On INIT- the server should disconnect not only the client but also close any open accepting code, fully shutdown the communication and send an INIT-. The quite code is only need for the IP layer in case the client disconnected. Then another client should be able to connect without special notifications on the FB's interface.
Re: Communication return codes enhancement proposal [message #1758223 is a reply to message #1758210] Fri, 24 March 2017 19:22 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

I had some time today in the train to further look at your code. I must confess a patch or a gerrit commit would have been easier to read as there only the changes would show up. But if I understood correctly you added the generic rClose function. As posted in my last comment I think this is a very good idea. I was shortly considering if we could replace your rCloseConnection with the destructor and save as an additional function call in commfb but I think the call order and vtable wouldn't be correct so the option you presented is sofar the best option. The only thing what I noticed is that if you give a default implementation of closeConnection in ComLayer the empty close connection would not be needed at all in asn1 and raw layer. Not sure what implication this then has.

Just one final thought on gerrit. The advantage what I learned reviewing contribution via gerrit in the last half year is that even if the code needs changes we can directly discuss with comments in the patch and then decie if it should be merged or a new commit overriding the old one is needed. SO maybe we should give it a try for your next suggestion.
Re: Communication return codes enhancement proposal [message #1758291 is a reply to message #1758223] Mon, 27 March 2017 08:26 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Alois Zoitl wrote on Fri, 24 March 2017 17:00
I think there is a misunderstanding with INIT- of the server. On INIT- the server should disconnect not only the client but also close any open accepting code, fully shutdown the communication and send an INIT-. The quite code is only need for the IP layer in case the client disconnected. Then another client should be able to connect without special notifications on the FB's interface.


I do understand this. When the client gets disconnected the external event is returning e_InitTerminated. The problem is that this check is being done outside the switch case structure, so when the server receives an INIT- itself he is also being quiet when he shouldn't. The fastest solution would be introducing this part inside the external events section.

Alois Zoitl wrote on Fri, 24 March 2017 19:22
I was shortly considering if we could replace your rCloseConnection with the destructor and save as an additional function call in commfb but I think the call order and vtable wouldn't be correct so the option you presented is sofar the best option. The only thing what I noticed is that if you give a default implementation of closeConnection in ComLayer the empty close connection would not be needed at all in asn1 and raw layer. Not sure what implication this then has.


Forcing layer implementations to declare the closeConnection and the openConnection may be desirable, even if they are empty, as this uniforms the API and makes the developer a bit more aware of what he can do. But the other approach (giving a default empty implementation) could also be useful removing the need of some lines of code in certain layers implementation. I think this topic is highly subjective. I don't see a clear winner myself.

Alois Zoitl wrote on Fri, 24 March 2017 19:22
Just one final thought on gerrit. The advantage what I learned reviewing contribution via gerrit in the last half year is that even if the code needs changes we can directly discuss with comments in the patch and then decie if it should be merged or a new commit overriding the old one is needed. SO maybe we should give it a try for your next suggestion.


I uploaded this suggestion too (first time using Gerrit, so if there's something wrong please tell me).
Re: Communication return codes enhancement proposal [message #1758295 is a reply to message #1758171] Mon, 27 March 2017 10:49 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Adrian Orive wrote on Fri, 24 March 2017 09:13

  1. Swap to a three enums scheme: connection, send and receive


As a proof of concept I have used this code to check how the following lines could be traslated as they were previously acting over a common type:
if(e_Nothing != eResp){
    STATUS() = scm_sResponseTexts[eResp & 0xF];
    QO() = !(eResp & scg_unComNegative);

    if(scg_unINIT & eResp){
        sendOutputEvent(scm_nEventINITOID);
    }
    else{
        sendOutputEvent(scm_nReceiveNotificationEventID);
    }
}


The checks are still easy to write and understand as they are and in my opinion they do not need to override some operator as I was suggesting before using three enums.

I also added an error return code for peer terminations for the IP layer. I think that the modbus and opc modules also return e_InitTerminated on peer problems right now so they should also swap to the new ones.
Re: Communication return codes enhancement proposal [message #1758306 is a reply to message #1758295] Mon, 27 March 2017 13:17 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

Thanks for submitting to gerrit this realy helps us to review the code and we get also an inital check if you have signed the Eclispe Contributor asignment. I noticed that you commited your change to the 1.8.x branch. This branch is for maintaining the 1.8. releases. As your proposed change is quite a big one I would find it better that we do this in the development branch, which is the branch for working on the next release. Normally Gerrit allows to move commits between branches. Therefore I have to aks you that you put your changes into the development branch and push them again to gerrit. Sorry for the inconvinience.

Reviewing your code a again stumbled across the rCloseConnection function, which has a rather unnice name. Do you have an idea if we can give it a better name?

I some got the idea if we should move the layer creation and deletion process completly to the comfb. so the rCloseConnection wouldn't be needed. But I'm not sure if this is a good idea. What do you think?

Yes your code snipped shows that the changes on that level are not disturbing and rather easy to handle. I noticed two minor things:

  1. scm_sResponseTexts[eResp & 0xF] shouldn't you use scm_sResponseTexts[eResp & scg_unCodeMask]
  2. with the new codeing guidlines you can ommit the type specifier for your variable names. This makes the code nicer to read and easer to maintain. For example scm_sResponseTexts would get scmResponseTexts, scg_unCodeMask would get scgCodeMask.



Re: Communication return codes enhancement proposal [message #1758366 is a reply to message #1758306] Tue, 28 March 2017 07:08 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Alois Zoitl wrote on Mon, 27 March 2017 13:17
Thanks for submitting to gerrit this realy helps us to review the code and we get also an inital check if you have signed the Eclispe Contributor asignment. I noticed that you commited your change to the 1.8.x branch. This branch is for maintaining the 1.8. releases. As your proposed change is quite a big one I would find it better that we do this in the development branch, which is the branch for working on the next release. Normally Gerrit allows to move commits between branches. Therefore I have to aks you that you put your changes into the development branch and push them again to gerrit. Sorry for the inconvinience.


Will push it as soon as I can.

Alois Zoitl wrote on Mon, 27 March 2017 13:17
Reviewing your code a again stumbled across the rCloseConnection function, which has a rather unnice name. Do you have an idea if we can give it a better name?



  • disconnect
  • deleteStack
  • removeStack
  • endCommunication
  • closeCommunication
  • Another combination involving one of the previous verbs and nouns


Alois Zoitl wrote on Mon, 27 March 2017 13:17
I some got the idea if we should move the layer creation and deletion process completly to the comfb. so the rCloseConnection wouldn't be needed. But I'm not sure if this is a good idea. What do you think?


I actually like the way it works now, as each layer is double-linked to the following and previous layers aside from the FB. This could also be done after creating all the layers but I think the actual way is more natural. The other way round could also be done probably and would allow to keep a reference to not only the top most layer at the FB but also the bottom most one, what will probably mean that the queue of interrupts could be simplified but this method will probably require some aditional auxiliar methods to properly double-link every layer (getters and setter for bottom and top layers), so the complexity will move to another part.

Alois Zoitl wrote on Mon, 27 March 2017 13:17
Yes your code snipped shows that the changes on that level are not disturbing and rather easy to handle. I noticed two minor things:

  • scm_sResponseTexts[eResp & 0xF] shouldn't you use scm_sResponseTexts[eResp & scg_unCodeMask]



The code in the post is extracted from the source, they are some of the lines that would need to be swapped. The code in the link is the "new" approach where the equivalent would be: (lines 41-50)

Alois Zoitl wrote on Mon, 27 March 2017 13:17

  • with the new codeing guidlines you can ommit the type specifier for your variable names. This makes the code nicer to read and easer to maintain. For example scm_sResponseTexts would get scmResponseTexts, scg_unCodeMask would get scgCodeMask.



Which convetion should I use?
Re: Communication return codes enhancement proposal [message #1758380 is a reply to message #1758366] Tue, 28 March 2017 08:27 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

Thanks for the new push. I just reviewed and successfully merged it.

From your name proposals I like disconnect the most. I think we can go without.

Adrian Orive wrote on Tue, 28 March 2017 07:08

Alois Zoitl wrote on Mon, 27 March 2017 13:17
I some got the idea if we should move the layer creation and deletion process completly to the comfb. so the rCloseConnection wouldn't be needed. But I'm not sure if this is a good idea. What do you think?


I actually like the way it works now, as each layer is double-linked to the following and previous layers aside from the FB. This could also be done after creating all the layers but I think the actual way is more natural. The other way round could also be done probably and would allow to keep a reference to not only the top most layer at the FB but also the bottom most one, what will probably mean that the queue of interrupts could be simplified but this method will probably require some aditional auxiliar methods to properly double-link every layer (getters and setter for bottom and top layers), so the complexity will move to another part.


The reson I suggested to move rClose code into ComFB is that then a layer does not need to know anything about shutdown or initilization orders. Also there is some code duplication in the closeConnection and createConnection code in the comFB and the according code in the layer. A noticed this when you had to change the rClose code on both sides. With a moveing everything to one place this could be omitted. But not sure if it is worth the effort.

For new code please follow the new codeing rules. I know this leads to some mixed code but ove time I think it will harmonize more and more.

Also it would be good if you could set your IDE to replace tabs with spaces and set the tab with to 2.
Re: Communication return codes enhancement proposal [message #1758391 is a reply to message #1758380] Tue, 28 March 2017 10:57 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Alois Zoitl wrote on Tue, 28 March 2017 08:27
Thanks for the new push. I just reviewed and successfully merged it.

From your name proposals I like disconnect the most. I think we can go without.


disconnect sounds fine. It appears 4 times in the whole project as shown in the image:

rCloseConnection() appearances

I have the commit ready but can't push it because the Change 93960 is closed.

Alois Zoitl wrote on Tue, 28 March 2017 08:27
Adrian Orive wrote on Tue, 28 March 2017 07:08

Alois Zoitl wrote on Mon, 27 March 2017 13:17
I some got the idea if we should move the layer creation and deletion process completly to the comfb. so the rCloseConnection wouldn't be needed. But I'm not sure if this is a good idea. What do you think?


I actually like the way it works now, as each layer is double-linked to the following and previous layers aside from the FB. This could also be done after creating all the layers but I think the actual way is more natural. The other way round could also be done probably and would allow to keep a reference to not only the top most layer at the FB but also the bottom most one, what will probably mean that the queue of interrupts could be simplified but this method will probably require some aditional auxiliar methods to properly double-link every layer (getters and setter for bottom and top layers), so the complexity will move to another part.


The reson I suggested to move rClose code into ComFB is that then a layer does not need to know anything about shutdown or initilization orders. Also there is some code duplication in the closeConnection and createConnection code in the comFB and the according code in the layer. A noticed this when you had to change the rClose code on both sides. With a moveing everything to one place this could be omitted. But not sure if it is worth the effort.


Something to keep in mind. A design question:

  • Would the FB then hold a reference to every layer?
  • Just to the top most one as now?
  • Or maybe both the top and bottom most ones so that it already has a reference to the bottom most for interrupts?



Alois Zoitl wrote on Tue, 28 March 2017 08:27
For new code please follow the new codeing rules. I know this leads to some mixed code but ove time I think it will harmonize more and more.


Which are the "new" codeing rules? I didn't found any in the Documentation.

Alois Zoitl wrote on Tue, 28 March 2017 08:27
Also it would be good if you could set your IDE to replace tabs with spaces and set the tab with to 2.


Done
Re: Communication return codes enhancement proposal [message #1758400 is a reply to message #1758391] Tue, 28 March 2017 12:31 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

Adrian Orive wrote on Tue, 28 March 2017 10:57

I have the commit ready but can't push it because the Change 93960 is closed.


As I accepted the change please make an independant commit with new change id.

Adrian Orive wrote on Tue, 28 March 2017 10:57

Something to keep in mind. A design question:

  • Would the FB then hold a reference to every layer?
  • Just to the top most one as now?
  • Or maybe both the top and bottom most ones so that it already has a reference to the bottom most for interrupts?


I would just keep the top layer for now. A simple getBottomLayer() function could be added to the layer allowing the comfb to iterate through the list.

The advantage of the interrupt list is that may makes sense also for intermediate layers to perform an interrupt to the comfb. Although I must confess we didn't had this sofar. But it could be leveraged when we finally imlementing zero copy comm stacks (i.e., the memory of the top or bottom layer is used for all data of an packet). In such a case the interrupting layer may be the fbdk asn1 layer. But this would definitly require a bigger redesign.

Adrian Orive wrote on Tue, 28 March 2017 10:57

Alois Zoitl wrote on Tue, 28 March 2017 08:27
For new code please follow the new codeing rules. I know this leads to some mixed code but ove time I think it will harmonize more and more.

Which are the "new" codeing rules? I didn't found any in the Documentation.

Ah forgot to mention the coding rules are in the docs directory of FORTE. Maybe we should also move them to our official documentation. Good pointer never thought about that.
Re: Communication return codes enhancement proposal [message #1758525 is a reply to message #1758400] Wed, 29 March 2017 15:05 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Alois Zoitl wrote on Tue, 28 March 2017 12:31
Adrian Orive wrote on Tue, 28 March 2017 10:57

Something to keep in mind. A design question:

  • Would the FB then hold a reference to every layer?
  • Just to the top most one as now?
  • Or maybe both the top and bottom most ones so that it already has a reference to the bottom most for interrupts?


I would just keep the top layer for now. A simple getBottomLayer() function could be added to the layer allowing the comfb to iterate through the list.


I've submitted a commit that moves the recursiveness to the FB. Tell me what you think about it. Be warned that I didn't have time to test it as much as I would like.

Alois Zoitl wrote on Tue, 28 March 2017 12:31
Adrian Orive wrote on Tue, 28 March 2017 10:57

Alois Zoitl wrote on Tue, 28 March 2017 08:27
For new code please follow the new codeing rules. I know this leads to some mixed code but ove time I think it will harmonize more and more.

Which are the "new" codeing rules? I didn't found any in the Documentation.

Ah forgot to mention the coding rules are in the docs directory of FORTE. Maybe we should also move them to our official documentation. Good pointer never thought about that.


Got it, I think my new changes comply with them.
Re: Communication return codes enhancement proposal [message #1758541 is a reply to message #1758525] Wed, 29 March 2017 20:05 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

Thanks for the next patch. A quick look shows that it will do the work quite nicely and now everything is in one place. We could also consider to move the parameter seperation back to comfb.

What I notieced is that you have in your while loop several returns and according error handling statements. I noticed that compilers normally generate smaller code if there is only one return. Furhtermore i personaly somethimes think that in the end the code ets also better readable. For example for the while you could add a second check to the while condition if the retval is still eInitOk and after the while an if (!eInitOk) for cleaning up. what do you think?
Re: Communication return codes enhancement proposal [message #1758556 is a reply to message #1758541] Thu, 30 March 2017 06:55 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Is it possible to submit a second commit to the samge gerrit? I mean not one that overrides the previous one, but that extends it. Or should I overwrite the actual one?

Can I use break statements to get out of the loop instead of adding a second condition to the while? I think the code stays clearer if you do this approach as errors are handled inside if statements but the main execution line stays flat instead of having to be implemented inside the if statement (or else). As an example:

while(Condition) {
  RetVal = doSomething();
  if("OK" != RetVal){
    // Handle specific error 1
    break;
  }

  RetVal = doSomethingElse();
  if("OK" != RetVal){
    // Handle specific error 2
    break;
  }

  RetVal = doLastThing();
  if("OK" != RetVal){
    // Handle specific error 3
    break;
  }
}

if("OK" != RetVal){
  // Handle all errors
}

return RetVal;
vs
while((Condition) && ("OK" == RetVal)) {
  RetVal = doSomething();
  if("OK" == RetVal){

    RetVal = doSomethingElse();
    if("OK" == RetVal){

      RetVal = doLastThing();

      if("OK" != RetVal){
        // Handle specific error 3
      }
    }
    else{
      // Handle specific error 2
    }
  }
  else {
    // Handle specific error 1
  }
}

if("OK" != RetVal){
  // Handle errors (common)
}

return RetVal;
Re: Communication return codes enhancement proposal [message #1758561 is a reply to message #1758556] Thu, 30 March 2017 07:05 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
And another question: what about moving extractLayerIdAndParams() to CComLayersManager. I think it may have sense.

EDIT: this could also be applied to buildIDString(). getDefaultIDString() should stay at the FB as it is overwritable.

[Updated on: Thu, 30 March 2017 07:17]

Report message to a moderator

Re: Communication return codes enhancement proposal [message #1758599 is a reply to message #1758561] Thu, 30 March 2017 13:07 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

I think I didn't put my issue very clear here. The problem I have is that error handling happens at three places. If there is a change in error handling it can happen that it is only fixed in one place. With a structure as proposed below error handling happens in one place. For me this is mostly better readable and has also less code.

CComLayer *previousLayer = 0; // Reference to the previous layer as it needs to set the bottom layer
  char *layerParams;
  EComResponse retVal = e_InitOk;
  // Iterate until reaching the end of the ID
  while(('\0' != remainingID) && (e_InitOk != retVal)){
    retVal = e_InitInvalidId;
	// Get the next layer's ID and parameters
	char *layerID = CComLayer::extractLayerIdAndParams(&remainingID, &layerParams);
	// If not well formated ID return an error
	if((0 != remainingID) && ('\0' != *layerID)){
		// Create the new layer
		CComLayer *newLayer = CComLayersManager::createCommunicationLayer(layerID, 0, this);
		// If the layer can't be created return an error
		if(0 != newLayer){
			// Open the layer connection
			retVal = newLayer->openConnection(layerParams);
			// Assign the newly created layer either to the previous layer or the FB
			if(0 == m_poTopOfComStack){
			  m_poTopOfComStack = newLayer;
			} else {
			  previousLayer->setBottomLayer(newLayer);
			}
			// Update the previous layer reference
			previousLayer = newLayer;			
		}		
	}
  }
  
  if(e_InitOk != retVal){
    closeConnection();
  }
Re: Communication return codes enhancement proposal [message #1758601 is a reply to message #1758599] Thu, 30 March 2017 13:34 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
I did understood your point, I was asking if a flat structure such as the following would be ok, to avoid multi-nested if(...){...}else{...} structures. The code will be the following:

EComResponse CCommFB::openConnection(){
  EComResponse RetVal;
  if(0 == m_poTopOfComStack){
    // Get the ID
    char *RemainingID;
    if(0 == strchr(ID().getValue(), ']')){
      RemainingID = getDefaultIDString(ID().getValue());
    }
    else{
      RemainingID = new char[strlen(ID().getValue()) + 1];
      strcpy(RemainingID, ID().getValue());
    }

    // If the ID is empty return an error
    if('\0' == *RemainingID){
      RetVal = e_InitInvalidId;
    }
    else {
      CComLayer *NewLayer;
      CComLayer *PreviousLayer; // Reference to the previous layer as it needs to set the bottom layer
      char *LayerParams;
      char *LayerID;
      // Loop until reaching the end of the ID
      while('\0' != RemainingID){
        // Get the next layer's ID and parameters
        LayerID = CComLayer::extractLayerIdAndParams(&RemainingID, &LayerParams);
        // If not well formated ID return an error
        if((0 == RemainingID) && ('\0' != *LayerID)){
          RetVal = e_InitInvalidId;
          break;
        }

        // Create the new layer
        NewLayer = CComLayersManager::createCommunicationLayer(LayerID, 0, this);
        // If the layer can't be created return an error
        if(0 == NewLayer){
          RetVal = e_InitInvalidId;
          break;
        }

        // Open the layer connection
        RetVal = NewLayer->openConnection(LayerParams);
        // If it was not opened correctly return the error
        if(e_InitOk != RetVal){
          delete NewLayer;
          break;
        }

        // Assign the newly created layer either to the previous layer or the FB
        if(0 == m_poTopOfComStack){
          m_poTopOfComStack = NewLayer;
        } else {
          PreviousLayer->setBottomLayer(NewLayer);
        }
        // Update the previous layer reference
        PreviousLayer = NewLayer;
      }
    }

    // If any error is going to be returned, delete the layers that were created
    if(e_InitOk != RetVal){
      closeConnection();
    }
  }
  else{
    // If the connection was already opened return ok
    RetVal = e_InitOk;
  }
  return RetVal;
}


As you can see, there are some if(...){...}else{...} structures but outside the while(...){...} block. Once inside that block, only one layer of if(...){...} is used to check for errors breaking the loop and handling it outside, thus returning only once in the function.

[Updated on: Thu, 30 March 2017 13:37]

Report message to a moderator

Re: Communication return codes enhancement proposal [message #1758618 is a reply to message #1758601] Thu, 30 March 2017 15:32 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
A couple of additional questions, one new and another unasnwered:

  1. What about exceptions? Would you agree to swan return enums to exceptions?
  2. Do you think that extractLayerIdAndParams() and buildIDString() fit more inside CCommFB or CComLayersManager?

Re: Communication return codes enhancement proposal [message #1758619 is a reply to message #1758601] Thu, 30 March 2017 15:32 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Sorry, the previous post got double-posted. You can delete this safely.

[Updated on: Thu, 30 March 2017 15:34]

Report message to a moderator

Re: Communication return codes enhancement proposal [message #1758623 is a reply to message #1758619] Thu, 30 March 2017 16:34 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

Oh sorry for missing the question.

I'll try to answer all your points. Regarding your code vs. mine I think it is mainly a matter of taste. I think both are fine. But it also relates a bit to your question about exceptions. Currently I'm more in favor of return values then exceptions and there are two main reasons the first one is that especially older compilers (which we still have for some platforms) tended to produce bigger/slower code with more memory usage when exceptions are enabled. Therefore we typically deactivate exceptions when building forte. Secondly writing robust reactive code using exceptions is also not as easy. Espcially as in systems like FORTE you want to be always active and always handle exceptions such that the overall forte is in a correct state as good as possible. I experience this now when working on 4diac-ide where we have exceptions but catching them in the wrong place may lead also to unwanted situations. E.g. that a full system is not loaded because one connection is problematic. I can recomend the Exceptional C++ book series from Herb Suttner on this topic. But I'm also learning here and happy for any input.

Your design with the breaks is more like throwing exceptions. But before breaking (i.e, throwing) you have to ensure that you leave forte in a correct state (e.g., you have to delete not correctly initalized layer). While I'll try to always keep the system in a correct state. So that close will work. (And while writing this I noticed a few points that should be changed in the above code anyways. See below). If we are now moving the while loop into an seperate function (i.e., createCommStack()). The RetVal = e_InitInvalidId; break; would turn into a return e_InitInvalidId and look very much like a throw statement.

So coming back to my point above. The delete in one of the layers. This is also something addressed in Herb Suttners books. How to manage resources. If we move the whole top/bottomlayer setting into the constructor of CommLayer the layer would be correctly inserted into the double linked list and even if there would be issues in opening the connection close connection would always correctly clean-up all the memory. I forgot how Herb called such a design but when using exceptions it gets even more important to not create memory or other resource leaks (e.g., open file handles).

You are right with not putting extractLayerIdAndParams() and buildIDString() to CommFB. This class is already quite long. CComLayersManager seams to be a good place for them.

Finally I want to thank you for all your questions and proposals. They are very inspiring.
Re: Communication return codes enhancement proposal [message #1758653 is a reply to message #1758623] Fri, 31 March 2017 09:46 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
This is going to be a long answer, sorry in advance.

Alois Zoitl wrote on Thu, 30 March 2017 16:34
I'll try to answer all your points. Regarding your code vs. mine I think it is mainly a matter of taste. I think both are fine. But it also relates a bit to your question about exceptions. Currently I'm more in favor of return values then exceptions and there are two main reasons the first one is that especially older compilers (which we still have for some platforms) tended to produce bigger/slower code with more memory usage when exceptions are enabled. Therefore we typically deactivate exceptions when building forte. Secondly writing robust reactive code using exceptions is also not as easy. Espcially as in systems like FORTE you want to be always active and always handle exceptions such that the overall forte is in a correct state as good as possible. I experience this now when working on 4diac-ide where we have exceptions but catching them in the wrong place may lead also to unwanted situations. E.g. that a full system is not loaded because one connection is problematic. I can recomend the Exceptional C++ book series from Herb Suttner on this topic. But I'm also learning here and happy for any input.


About the "difficulty" of using exceptions, I think most of it depends on the context you come from. For example, languages such as C# or Python use them so a backgraound in these languages trains you in the appropiate mindset. C, on the contrary, handles this kind of situations with return codes and usually C coders feel more confortable with return codes than exceptions.

About old compilers support, that is a fully understandable major design decission. I would just like to add that exceptions are usually critized of being a bigger overhead that return codes, and this is actually a bit of a lie. Code handling exceptions on normal execution has near to zero overhead. When exceptions occur, this may not be true and return codes could be better performance-wise in these scenarios, but exceptions offer other advantages such as being a class and being able to store information about the state when the exception was raised, the place in the code where it was thrown, etc. Keep in mind that most exceptions mean that somethign went wrong so the strict performance is not as important as in normal operations. Exceptions are also the only way of controlling Constructors execution as you can not return a enum/int from them.

Alois Zoitl wrote on Thu, 30 March 2017 16:34
Your design with the breaks is more like throwing exceptions. But before breaking (i.e, throwing) you have to ensure that you leave forte in a correct state (e.g., you have to delete not correctly initalized layer). While I'll try to always keep the system in a correct state. So that close will work. (And while writing this I noticed a few points that should be changed in the above code anyways. See below). If we are now moving the while loop into an seperate function (i.e., createCommStack()). The RetVal = e_InitInvalidId; break; would turn into a return e_InitInvalidId and look very much like a throw statement.


Yes, it looks similar to to exception handling in the way that the return code is assigned and then the while loop is broken. This is the reason I asked about them (exceptions) as I didnt' know the "avoid exceptions due to old compiler support/performance issues" design principle.

About the delete NewLayer; instruction, it is not really needed. NewLayer is created inside the openConnection() function, and if fullfilling that if condition it won't be assigned neither to m_poTopOfComStack nor to PreviousLayer, thus when leaving the openConnection() function, the resource will be released and garbage collected. Doing it before just deletes it before so that the resources are freed earlier. It could also be added in your code with the same purpose as both of us are calling closeConnection() afterwards and this may take a while (it has nothing to do with closeConnection() itself, but with the time it takes before getting to the end of the function so that the scopes changes and the NewLayer item is automatically deleted).

Alois Zoitl wrote on Thu, 30 March 2017 16:34
So coming back to my point above. The delete in one of the layers. This is also something addressed in Herb Suttners books. How to manage resources. If we move the whole top/bottomlayer setting into the constructor of CommLayer the layer would be correctly inserted into the double linked list and even if there would be issues in opening the connection close connection would always correctly clean-up all the memory. I forgot how Herb called such a design but when using exceptions it gets even more important to not create memory or other resource leaks (e.g., open file handles).


Does RAII ring a bell? I think it may be what you are talking about.

About using the Constructor as the tool to build the whole stack, this has to be done carefully. Bear in mind that Constructors can't return codes as they return the object itself, so potentially error-prone operations should not be done in the constructor, just memory allocation and so on. After saying this, the constructors could be used for the double-linking process, yes, but I still think the openConnection() function is needed. Right now a layers ID and params are being extracted, the layer is being constructed and the connection is being opened before jumping to the next layer, if there is another one. This process could be split into two parts. First all the layers could be created by extracting the params and creating the item and after all are created, the connections of all of them will be called in order. This approach actually makes sense too but remember that Constructors can't return codes, so they must never result in an error (unless you move to exceptions). This will mean that the ID should probably be parsed completely, obtaining a queue of ids and a second queue of params, using the first to call the CComLayer constructors and the second queue to call the openConnection() functions. This parsing the ID will be done before the Constructor calls, so error handling could still be implemented without exceptions. This will make the construction process similar to the deconstruction suggested in the patch we are refering to, handled by destructors that call their bottom layer closeConnection() method before deleting it (and thus obtaining the recursive behaviour).

The resulting code will look something similar to:

EComResponse CCommFB::openConnection(){
  EComResponse RetVal;
  if(0 == m_poTopOfComStack){
    // Get the ID
    char *RemainingID;
    if(0 == strchr(ID().getValue(), ']')){
      RemainingID = getDefaultIDString(ID().getValue());
    }
    else{
      RemainingID = new char[strlen(ID().getValue()) + 1];
      strcpy(RemainingID, ID().getValue());
    }

    // Parse ID into two deque, one containing layer IDs (RemainingIDs) and the second layer params (RemainingParams).

    char *LayerID;
    strcpy(LayerID, RemainingIDs.front());
    RemainingIDs.pop_front();
    m_poTopOfComStack = CComLayersManager::createCommunicationLayer(LayerID, RemainingIDs, 0, this);

    if(0 != m_poTopOfComStack){
      Layer = m_poTopOfComStack;
      RetVal = e_InitOk;
    }
    while((e_InitOk == RetVal) && (0 != Layer)){
      RetVal = Layer->openConnection(RemainingParams.front());
      RemainingParams.pop_front()
      Layer = Layer->getBottomLayer();
    }

    // If any error is going to be returned, delete the layers that were created
    if(e_InitOk != RetVal){
      closeConnection();
    }
  }
  else{
    // If the connection was already opened return ok
    RetVal = e_InitOk;
  }
  return RetVal;
}

CComLayer::CComLayer(std::deque RemainingIDs, CComLayer* pa_poUpperLayer, CCommFB * pa_poComFB) :
  m_eConnectionState(forte::com_infra::e_Disconnected), m_poTopLayer(pa_poUpperLayer), m_poBottomLayer(0), m_poFb(pa_poComFB){
  if(!RemainingIDs.empty()){
    char *LayerID;
    strcpy(LayerID, RemainingIDs.front());
    RemainingIDs.pop_front();
    m_poBottomLayer = CComLayersManager::createCommunicationLayer(LayerID, RemainingIDs, this, m_poFb);
  }
}


NOTE 1: CComLayersManager::createCommunicationLayer() is not exactly a Constructor, but a factory function that calls a Constructor, this has some implications, as IIRC the factory function is returning 0 when he can't find which Constructor to call. This 0 will get assigned to CComLayer's m_poBottomLayer silently stopping the stack creation as if it had reached the end when it finds an ID that doesn't have Constructor. This means that, either every ID in the deque has constructor or the size of the params deque at the end is zero, needs to be checked.
NOTE 2: if we swap the two last CComLayersManager::createCommunicationLayer() and CComLayer::CComLayer() parameters we could assign a default value of 0 to the top layer.
NOTE 3: extracting the next ID from the deque that is done in two places could be moved inside CComLayersManager::createCommunicationLayer().
NOTE 4: parsing the ID into the two deque objects should take into account error codes.
Re: Communication return codes enhancement proposal [message #1758663 is a reply to message #1758653] Fri, 31 March 2017 11:47 Go to previous messageGo to next message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

Thanks for your long and detailed reply. It has quite some food for thought, which I would definitly like to digest and consider for future 4diac design points. If you have a good pointer for learning more how to effectily make use of exceptions I would be greatfull. I think this would help me for me work in the ide and who knows maybe in future also for FORTE.

There is one furter rule we have in FORTE for the code in core: we are not using any STL libraries. The reason is first that on some embedded boards or operating systems we don't have it available and secondly STL pulls in quite a lot which needs quite some RAM and FLASH which is an issue on the smallest paltform. I know that this means sometimes not so nice C++ code and therefore if you are developing modules which are targeting Linux based systems I definitly would recomend to use STL there to your advantage but please for now not in core. I also hope this will change in the future.

I think I was not clear enough with saying build up the double linked list in CommLayers constructor. As you pointed out Constructors are very bad regarding error reporting. Therefore I didn't mean to put the createCommunicationLayer there as we don't have a good way of checking if that worked without exceptions (you are right here would be a good place for exceptions). Also it would be similar as we have it now with the createCommunicationLayer in two places. I thought more of changing the construcutor of the CComLayer to the following:

CComLayer::CComLayer(std::deque RemainingIDs, CComLayer* paUpperLayer, CCommFB * paComFB) :
  m_eConnectionState(forte::com_infra::e_Disconnected), m_poTopLayer(paUpperLayer), m_poBottomLayer(0), m_poFb(paComFB){
  if(0 != paUpperLayer){
    paUpperLayer->setBottomLayer(this);   
  }
}


And keep the loop in CComFB as we had it above. So id parsing and layer creation would be triggered from one common place. But as soon as a new layer is created by the factory it would be in the linked list and close connection would always free the memory. As postilated in the RAII principle. Although I thought there is a second. Which has something todo with a tranactional behavior. Nevermind.
Re: Communication return codes enhancement proposal [message #1758667 is a reply to message #1758663] Fri, 31 March 2017 12:55 Go to previous messageGo to next message
Adrian Orive is currently offline Adrian OriveFriend
Messages: 38
Registered: March 2017
Member
Alois Zoitl wrote on Fri, 31 March 2017 11:47
There is one furter rule we have in FORTE for the code in core: we are not using any STL libraries. The reason is first that on some embedded boards or operating systems we don't have it available and secondly STL pulls in quite a lot which needs quite some RAM and FLASH which is an issue on the smallest paltform. I know that this means sometimes not so nice C++ code and therefore if you are developing modules which are targeting Linux based systems I definitly would recomend to use STL there to your advantage but please for now not in core. I also hope this will change in the future.


Ok, we could still implement ourselves a deque-like struct, or just do some string conversion: "ID1[Param11, Param12].ID2[Param21].ID3[]" ==> "ID1.ID2.ID3" and "[Param11, Param12][Param21][]" that could be properly interpreted.

Alois Zoitl wrote on Fri, 31 March 2017 11:47
I think I was not clear enough with saying build up the double linked list in CommLayers constructor. As you pointed out Constructors are very bad regarding error reporting. Therefore I didn't mean to put the createCommunicationLayer there as we don't have a good way of checking if that worked without exceptions (you are right here would be a good place for exceptions). Also it would be similar as we have it now with the createCommunicationLayer in two places. I thought more of changing the construcutor of the CComLayer to the following:

CComLayer::CComLayer(std::deque RemainingIDs, CComLayer* paUpperLayer, CCommFB * paComFB) :
  m_eConnectionState(forte::com_infra::e_Disconnected), m_poTopLayer(paUpperLayer), m_poBottomLayer(0), m_poFb(paComFB){
  if(0 != paUpperLayer){
    paUpperLayer->setBottomLayer(this);   
  }
}


And keep the loop in CComFB as we had it above. So id parsing and layer creation would be triggered from one common place. But as soon as a new layer is created by the factory it would be in the linked list and close connection would always free the memory. As postilated in the RAII principle. Although I thought there is a second. Which has something todo with a tranactional behavior. Nevermind.


I actually like this, will apply it to che commit.
Re: Communication return codes enhancement proposal [message #1758779 is a reply to message #1758667] Sun, 02 April 2017 19:48 Go to previous message
Alois Zoitl is currently offline Alois ZoitlFriend
Messages: 1560
Registered: January 2014
Senior Member

In utils there is an array template and a fixed capacity vector using this array template. Maybe they can be of help for implementing an own deque. In the other side I'm not sure if it is worht the effort given the existing code and that it is currently only used in one place.
Previous Topic:mqtt
Next Topic:ModBus Protocol
Goto Forum:
  


Current Time: Thu Mar 28 18:00:22 GMT 2024

Powered by FUDForum. Page generated in 0.07966 seconds
.:: Contact :: Home ::.

Powered by: FUDforum 3.0.2.
Copyright ©2001-2010 FUDforum Bulletin Board Software

Back to the top