MBP-389: Monitoring crash - #52
Conversation
federrg
left a comment
federrg
left a comment
There was a problem hiding this comment.
Implement suggested changes proposed by @szilard-ess
|
I have added clear of aSlaveConfig if the current slave count is less than, the count in the previous cycle to not have invalid data stored inside aSlaveConfig from previous copy. |
There was a problem hiding this comment.
Chages requsted by me have been implemented. This is a bug fix, if the bug has been fixed then this can be approved.
I am the one who requeted the changes, so I guess I need to approve. so just waiting for the ocnfimration from @mac-kan @szilard-ess tha the bug has benn fixed.
With the latest changes, there is no more MEMSET/MEMMOVE use which could of caused issues as @mac-kan mentioned it, instead we swap between the 2 buffers.
I had this note previously, but in commit a1d4c76 I changed the delay time to be calculated based on the number of slaves, this makes sure that we have enough time to read the slaves atleast once. Another solution would be to have bFirstReadDone variable which is written after the first swap of the pointers. |
|
@szilard-ess I do not have a strong perference. If the variable will facilitate debugging and trouble shooting by all means add it. if not at least a comment in the code explaining what you metioned would be enough. I leave it up to your criteria. |
If I understand the old code fully, the timer fbEtherCATAlarmDelayTON is there to delay the alarms and wait for the read of fbGetEcSlaveStates. If there is no other reason, then I would prefer a variable named bFirstReadDone, instead of a timer. @mac-kan could you please confirm this for me? |
|
Hi @szilard-ess , yes, that timer is there exactly for that reason. we were getting EtherCAT error every start up or activating configuration, so we included the delay. |
Thank you for the info, I changed it to wait until the first read is finished instead. |
There was a problem hiding this comment.
Nice job with changing the swap logic to pointers, and your suggestion of moving away from the time dependency when starting up! 👍
I think there is still some work required related to the guard logic though (see specific comment). Because of this, another bug seems to be masked, and I think it would make sense to introduce a startup timeout, so that we can trigger the '0 EtherCAT Slaves found' error after some seconds. As is now, that error should be triggered the cycle after the first failed read attempt during startup, then it will pop through. After the first issue has been fixed, the message will be prevented to pop through if happening during startup.
mac-kan
left a comment
mac-kan
left a comment
There was a problem hiding this comment.
Looks like most things have been fixed, good job!
Updates and enabled error message of slave link error, when no EtherCAT slave were found.
Reduces the amount of data being copied drung EtherCAT monitoring, which caused cycle time spikes.
Adds clear to aSlaveConfig if the current slave count is less than, the count in the previous cycle to not have invalid data stored inside aSlaveConfig from previous copy.
Uses read/write buffers to remove the use of MEMCOPY/MEMMOVE/MEMSET, which increased cycle time.
Fixes the timer of get slave config data to work as a delay after a finished read out instead of triggering execute every 100 ms, eventough the read function block was still busy.
Changes early return logic, checks when FB_EcGetConfSlaves is done with the first readout, instead of relying on a timer.
Changes order of early returns to be able print error message even when there was no sucessful read at all.
Adds clear of zero slave found error to be able to detect it more than once.
Fixes the issue of possible buffer swap without sucessful read of configured slave data, by introducing the check of returned slave count.
Changes execute logic of all diagnostic function blocks to start the execution on startup and to have a consistent logic.
Adds init of buffer addresses, to fix initialization restriction of variables inside VAR_INST.
b8717e3 to
482a15f
Compare
3 participants
Fixes the infinite loop inside monitoring base with an early return. This caused an issue, that would crash the IPC. Also logs an error message when no EC slaves found.