OpenEtherCATsociety / SOES

Simple Open Source EtherCAT Slave
Other
578 stars 249 forks source link

ESC_ALstatus changes don't trigger post_state_change_hook #45

Closed mheden closed 5 years ago

mheden commented 5 years ago

It seems like calls to ESC_ALstatus don't trigger state hooks.

For example in the demo application:

      if (watchdog <= 0)
      {
         DPRINT("DIG_process watchdog expired\n");
         ESC_stopoutput();
         /* watchdog, invalid outputs */
         ESC_ALerror (ALERR_WATCHDOG);
         /* goto safe-op with error bit set */
         ESC_ALstatus (ESCsafeop | ESCerror);

This will not trigger the post_state_change_hook which I think it should.

I think it is caused by ESC_state are looking at ALcontrol rather than ALstatus. On the other hand, I'm not really sure of how it should be handled since ESC_state is setting ALstatus if the transition is successful.

Maybe ESC_ALstatus should call hooks as well?

nakarlsson commented 5 years ago

In what scenario are you lacking hook support?

As implemented, ESC_state only act on requests from the master, which is the only normal way to trigger a state change except when triggering an error and doing the error transition OP->SAFEOP|E. So, doing a request by the slave ALStatus state change would only make sense/be valid when there is an error, right?

I second what you say above on ESC_state, would an alternative way be to add a trigger ERROR function, going to new state, set error and integrate some hook, eg. post state change hook. I believe this could be handy if you would like to trigger an error from outside the "stack" instead of calling the sequence above for the watchdog.

mheden commented 5 years ago

Yes, in my case I wanted to be informed about watchdog timeouts. Since I react to all other transitions via the post_state_change_hook I think it would make sense to also call that on "internal stack errors" so there is "one point of information".

nakarlsson commented 5 years ago

Could you create an exmaple that would cover your needs?

nakarlsson commented 5 years ago

@mheden , check the follwoing proposal at feature/esc_go_to_errorstate

The user/application should use it / watchdog, invalid outputs goto safe-op with error bit set / ESC_ALstatusgotoerror((ESCsafeop | ESCerror), ALERR_WATCHDOG);

nakarlsson commented 5 years ago

ping @mheden, Opinion, should ESC_ALstatusgotoerror also ESC_stopoutput , can be conditional on OP?

mheden commented 5 years ago

Sorry, missed this.

Looks good to me. I think it makes sense to call ESC_stopoutput from ESC_ALstatusgotoerror since all errors should make state transitions from OP. Or are there any errors that can appear together with OP?

nakarlsson commented 5 years ago

Don’t think so.