search

OpenEtherCATsociety / SOES

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

pass AL event mask to the interrupt worker thread instead of hard coding it #33

Closed nakarlsson closed 6 years ago

nakarlsson commented 6 years ago

It's an API change, so technically it should become v3.0.0, but I suggest we make this v2.0.1 and make it superseded v2.0.0 as main 2.0 release. Any thoughts?

Alternative is to create new function with the mask as argument and keep the old as is.

ArthurKetels commented 6 years ago

Although it is an API change. I doubt there are many users affected. And I like the concept. So skip the wrapper function. Now the ISR function call is much more explicit. The next obvious step is to add some locking and have the event handling split in an ISR and main polling function. Specially the eeprom handling in ISR is a bit ackward.

nakarlsson commented 6 years ago

I guess _isr might mislead people but my intention was that it should handle the non-synchronous (AL Ctrl, SM change, SM0, SM1 and EEP) interrupts, making it an interrupt handler, either from a back-ground task or from an ISR, the later make no sense as you say unless you allow nested PDI interrups.

Should we rename the function ecat_slv_isr -> ecat_slv_worker?

In the rt-kernel-xmc4\rtl_xmc4_irq example it is started from the XMC esc_hw.c ISR and run from a background task. The target applications are those that are driven by interrupts only and will idle between SM2/DC interrupts during normal operations. For non SPI slaves no need for locking as long as single variables are handled in an Atomic way.

Applications polling for non-synchronous interrupts should use ecat_slv_poll() that relay on polling ALEvent in ESC_read/ESC_write.

Applications running legacy free-run mode should use ecat_slv() (old soes())

I've attached a draft of the SOES refmanual

soes_refman_draft.pdf

ArthurKetels commented 6 years ago

Should we rename the function ecat_slv_isr -> ecat_slv_worker?

Seems like the right thing to do. The application can choose the proper context to execute the worker function.

nakarlsson commented 6 years ago

I added a _worker function and deprecated the _isr one, so everything should be by the book. Bump to 2.1.0 to cover the deprecate.

Before proceeding with the API change. One final thought was to re-use the concept from the documentation, basically there are 3 modes, polling / mixed polling & interrupt / interrupt

ecat_slv_interrupt ( Old ecat_slv_isr / ecat_slv_worker ) ecat_slv_mixed_poll ( Old ecat_slv_poll ) ecat_slv_poll ( Old ecat_slv )

Should we keep it as the current PR or use the above or do you have a better suggestion?

ArthurKetels commented 6 years ago

I would keep it at lean as possible. Introducing more intermediate layers without real functionallity is more confusing than helpfull in my mind. I have a preference for the current PR.