minor_improvement/ ecx_readstate() and/or ecx_statecheck() improved?

[MagnaboscoL]

Applied required changes.

[nakarlsson]

What is the use case you have in mind?

I have 2 concerns with this update

1. It iterate twice over all slaves (in theory 65k x 2)
2. It only updates slaves status if all are in the same state else it don't. Which makes me think it shouldn't update at all as of today.

[MagnaboscoL]

I understand your concerns,
here you have what I think about them:

1. as you pointed out iterate twice is not the better solution, but:

• the code could be improved to iterate once
• this is a blocking function which duration mostly depend on how much time slaves need to change state. Does the delay introduced by the "for loop" really matter here? Isn't this function used mainly during the configuration phase?

2. The use case I have in mind is:
   Suppose the function ecx_statecheck(context, 0, EC_STATE_OPERATIONAL, EC_TIMEOUTSTATE); is called by user's code and return true.
   Just few milliseconds after user's code checks ec_slave[1].state and there are two possibilities :

• User's code is designed to call ecx_readstate() before reading ec_slave[1].state and and this causes a number of datagrams equal to slaves number to be sent. Would it be efficient?
• User thinks that, since ec_checkState(...) has been called with the second argument equal to 0, all the slaves already have ec_slave[x].state updated. We know this is not the case and probably a previous state would be read.
  Does it seems an expected behaviour from user point of view?

3. Yes, the function as I have proposed would update the state only when possible without sending more datagrams. The truth is that I would have preferred to update ec_slave[x].state in any case (sending one datagram per slave if needed). Indeed if the function return false it seems very reasonable to me to check ec_slave[x].state of every slave, to find which one is in a different state.
   However I thought that probably this behaviour would have penalised the users that (aware of how the function was working before) are already calling ecx_readstate()after ecx_statecheck(...).

Some doubts about ecx_statecheck(...) current implementation:

1. When the original function is called with an argument different from 0 ec_slave[x].state of the correspondent slave is updated, while when called with argument 0 (that cause all slaves to be checked) ec_slave[x].state of no slave is updated. Moreover ec_slave[0].state is defined by SOEM as the lower slaves' state but if ec_slave[x].state isn't updated it could be lower than ec_slave[0].state.
   Do you think that this behaviour is consistent and easy to be understand from the user?

2. If the state of all slaves isn't updated even if there is the information to update it, contained in the broadcast datagram, doesn't it seems a waste of information content to you?

I am sorry if I have been too wordy,
I hope to have expressed clearly my opinion.

[MagnaboscoL]

If it is preferred to keep the function I would have the following considerations/questions:

1. In my opinion it should be clearly stated in the function description that it does not update the states of all the slaves if called with the argument "slave" set to 0.
2. In your opinion isn't important to keep ec_slave[x].state aligned to the real state of the slave as much as possible?
3. After calling ecx_statecheck(...) you think that ecx_readstate() should be called in any case? If yes, wouldn't be inefficient in your opinion to send a datagram for every slave even if this is not needed at all?
4. Is it clear that with the proposed implementation ecx_readstate() could be called only if ecx_statecheck(...) fails? (Of course should be detailed in the function description)
5. Shouldn't be improved at least the funtion ecx_readstate() so that at the beginning it tries to determine the state of all the slaves at once using a broadcast datagram? Sometimes it could save up to 65534 datagrams.

[nakarlsson]

1. Yes that could be clarified.
2. Yes but leave that to the user with read.
3. I think your 5. is the way to go, let the users how want to keep them up-tod-ate do it, and try with minimum effort.
4. Might be good to describe the two functions in comparison to each other.
5. Make sense, I think check should be left alone and read can be improved to try at first with a BRD.

[MagnaboscoL]

@nakarlsson Thanks!