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
   

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

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

• 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): -
  

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

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.

if(resp & e_Nothing) {...}
if(resp & e_Ok) {...}

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

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

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)
    

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) {...}

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.

• 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.
  

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
   

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 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.

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

if(e_Nothing != eResp){
    STATUS() = scm_sResponseTexts[eResp & 0xF];
    QO() = !(eResp & scg_unComNegative);

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

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.
   

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?

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

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:

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

• 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.
  

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.

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.

• 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?
  

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.

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

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.

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.

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.

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?
   

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).

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);
  }
}

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.