Use of GPIO interrupts in EX drivers

Discussions and support about ChibiOS/EX, the External Peripherals Abstraction Layer.
awygle
Posts: 32
Joined: Sun Apr 03, 2016 8:39 pm
Has thanked: 6 times
Been thanked: 4 times

Use of GPIO interrupts in EX drivers

Postby awygle » Wed May 25, 2016 2:19 am

Hi all,

I have a need to make use of GPIO interrupts in an EX driver for a radio transceiver that I'm working on, and I'm struggling to see the best way to go about that. Here are the approaches I've looked at:

1) Just write GPIO interrupts directly using hardware functionality. This is obviously wrong because then the EX driver won't be portable. It should depend on the other HAL modules only. It appears that EXT is the one to use - while I'm aware it's intended to do more than just GPIO interrupts, it does seem to have dominion over them.

2) Use the EXT module directly - that is, write an EXTConfig struct and call extStart from my EX driver. This is bad because it prevents the user of the EX driver from using the EXT driver, and also won't allow more than 1 EX driver to be used at a time.

3) Modify an already-running EXT module's config to add the required interrupts based on lines passed in with the config struct for the EX driver. This seemed like a good idea until I actually dug into the EXT module in more detail. Because of its abstraction of channels, there's not a 1-to-1 correspondence in EXT between channels and lines. So we can't say "channel 1 will be the FIFO Full line from the transceiver" because channel 1 might have been configured as something totally different already. We would need some kind of "extGetNextFreeChannel" to make this work.

4) Pass in an EXT channel (expchannel_t - and is that a typo by the way?) instead of, or in addition to, a line. This seems like an OK idea, and honestly I could even make it work, but only because I know from reading the low level driver sources that expchannel_t is always an integer type in current implementations. I'd have to violate the abstraction implied by the typedef to make this work. To expand further:

I could put fields like this in the transceiver driver's config struct:

Code: Select all

typedef struct {
  ...
/** @brief Channel to use to configure FIFO interrupt */
  expchannel_t fifo_channel;
  ...
} txvrConfig;


And I could put this in the transceiver driver's Start function:

Code: Select all

void txvrStart(txvrDriver *devp, txvrConfig *config) {
  ...
  /* Configure the FIFO interrupt channel */
  extSetChannelMode(&EXTD1, config->fifo_channel, &fifo_channel_config);
  ...
}


But when initializing the transceiver config struct I would have to use something like this:

Code: Select all

txvrConfig default_config = {
  ...
  15, /* fifo_channel */
  ...
}


and that feels like violating the abstraction. To make this work cleanly we would need some sort of extGetChannelByIndex(n) macro that in all existing cases just expands to n. Or we could drop the expchannel_t thing entirely and just pass around indexes instead of opaque identifiers.

5) Write the required interrupt callback functions in the EX driver and rely on the user to make sure they actually get set up in the EXT driver somewhere. I dislike this because it leaks out implementation details of the EX driver into user code.



After that big fat wall of text - are there better solutions to this problem? I dug around the forum somewhat and found a lengthy discussion here: viewtopic.php?f=3&t=3249&start=10 with no real resolution, as well as here: viewtopic.php?t=387&start=10 which proposed my solution #4 but still has the abstraction problem I discuss.

I think these kinds of problems are likely to continue to come up as we build out the EX project, it would be to all of our advantages to come up with a mutually acceptable way to solve them. For my money, my two favorites would be my solution #3, with the proposed extGetNextFreeChannel or similar, or rolling GPIO-specific interrupt functionality into the PAL (which is a major change) so that there is a 1-to-1 correspondence and I can use a line identifier to configure an interrupt on that line. Other opinions are of course actively sought, and obviously Giovanni it's ultimately your call.

Thanks for your time!

flabbergast
Posts: 71
Joined: Sat Aug 22, 2015 1:22 pm

Re: Use of GPIO interrupts in EX drivers

Postby flabbergast » Wed May 25, 2016 8:34 am

I personally like #5 the most. {E.g. the current USB-CDC 'driver' implementation works like that, you need to call a particular hook from the start-of-frame handler for the driver to work.}

Also, for instance on Kinetis, the EXT configuration structure doesn't need to contain anything else than the lines that are actually used; so to make something like #3 work, one would need to make the EXT driver to maintain a pool of *all possible* EXT channels... (or I may be misunderstanding the proposal).

awygle
Posts: 32
Joined: Sun Apr 03, 2016 8:39 pm
Has thanked: 6 times
Been thanked: 4 times

Re: Use of GPIO interrupts in EX drivers

Postby awygle » Wed May 25, 2016 8:44 am

Addressing your second paragraph (first is a matter of opinion) - actually what I am suggesting instead is having a pool of channels of size EXT_MAX_CHANNELS, which is user-configurable. Then when allocating a channel for the lines that are actually used, as you say, do so by calling a function extGetChannel(EXTChannelConfig *config) which will return an expchannel_t handle to the channel that is used for all subsequent manipulation. This is very very close to what the current Kinetis driver does, but removing the user visibility of the ordering implied by the "channels[EXT_MAX_CHANNELS]" array in the EXTConfig structure. That way we can have an order of events like this:

1) User configures their own channels by calling extGetChannel, say, four times. channels[0] through channels[3] are filled in and returned to the user one at a time.

2) My EX driver's start routine is called. It needs to use interrupts on two lines, so it calls extGetChannel twice and receives channels[4] and channels[5].

The key difference is that currently my EX driver would have to know that the user filled in specifically channels[0] through channels[3] in order to correctly fill in [4] and [5] rather than [0] and [1]. Think more allocating from a pool rather than filling out a predefined array. Eventually the end user can reduce EXT_MAX_CHANNELS to the number that are actually used.

User avatar
Giovanni
Site Admin
Posts: 12179
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 571 times
Been thanked: 488 times
Contact:

Re: Use of GPIO interrupts in EX drivers

Postby Giovanni » Wed May 25, 2016 12:23 pm

Hi,

EXT driver is scheduled for a radical change, the idea is to not configure channels any more as a static configuration but to enable/disable them individually with dynamic callbacks.

The idea is of integrate the functionality in the PAL driver and add something like:

Code: Select all

palLineEnableEvent(line, mode, callback/NULL)  // Callback or Cortex event.
palLineDisableEvent(line)


Non-GPIO EXTI lines would no more be handled and left to the application/other drivers (RTC for example). The current EXT driver will remain for compatibility but not actively ported to new devices.

My idea is to give this concept a try after finishing the Flash driver infrastructure.

Additional/alternative ideas are welcome, the concept is not frozen.

Giovanni

awygle
Posts: 32
Joined: Sun Apr 03, 2016 8:39 pm
Has thanked: 6 times
Been thanked: 4 times

Re: Use of GPIO interrupts in EX drivers

Postby awygle » Wed May 25, 2016 10:24 pm

I think that sounds great. Honestly I'd expected that a big change to such a core driver as PAL would be unacceptable, glad to see I was wrong :)

flabbergast
Posts: 71
Joined: Sat Aug 22, 2015 1:22 pm

Re: Use of GPIO interrupts in EX drivers

Postby flabbergast » Thu May 26, 2016 3:21 pm

This sounds very good, I am looking forward to it - I find the current EXT configuration system somewhat cumbersome, especially when moving between ports. It's great that now the complexity will be shifted from the end user to the port implementation.

User avatar
Korken
Posts: 270
Joined: Wed Apr 02, 2014 4:09 pm
Location: Luleå, Sweden
Has thanked: 5 times
Been thanked: 6 times
Contact:

Re: Use of GPIO interrupts in EX drivers

Postby Korken » Thu May 26, 2016 4:03 pm

With this it will be possible to support ISRs in the sensor codes as we discussed, so I am very happy with this decision! :D

awygle
Posts: 32
Joined: Sun Apr 03, 2016 8:39 pm
Has thanked: 6 times
Been thanked: 4 times

Re: Use of GPIO interrupts in EX drivers

Postby awygle » Tue May 31, 2016 5:25 am

Hi Giovanni, hi all,

I needed this so I, uh, wrote it :oops:. Built and tested with the proper support added to my MSP430X port's pal_lld - sorry but I don't have any STM32 devices to test on. Feel free to use it if you find it useful - if not, I'll build against my patched version until your final version goes live and then fix whatever breaks.

You might also want to put this typedef in the hal_pal.h - I kept it in the lld to match other patterns I found but it seems pretty universal to me:

typedef void (*palcallback_t)(ioportid_t port, uint8_t pad);

Related: I tried to attach this patch to the forum post but it didn't like the extension .patch or the extension .txt. Is that a bug, or intended behavior?

Code: Select all

diff --git a/os/hal/include/hal_pal.h b/os/hal/include/hal_pal.h
index 3df8414..bc3a373 100644
--- a/os/hal/include/hal_pal.h
+++ b/os/hal/include/hal_pal.h
@@ -98,6 +98,20 @@
 #define PAL_HIGH                        1U
 /** @} */
 
+/**
+ * @name    PAL event modes
+ * @{
+ */
+#define PAL_EVENT_MODE_EDGES_MASK   3U  /**< @brief Mask of edges field.    */
+#define PAL_EVENT_MODE_DISABLED     0U  /**< @brief Channel disabled.       */
+#define PAL_EVENT_MODE_RISING_EDGE  1U  /**< @brief Rising edge callback.   */
+#define PAL_EVENT_MODE_FALLING_EDGE 2U  /**< @brief Falling edge callback.  */
+#define PAL_EVENT_MODE_BOTH_EDGES   3U  /**< @brief Both edges callback.    */
+
+#define PAL_EVENT_MODE_AUTOSTART    4U  /**< @brief Channel started
+                                             automatically on driver start. */
+/** @} */
+
 /*===========================================================================*/
 /* Driver pre-compile time settings.                                         */
 /*===========================================================================*/
@@ -530,6 +544,49 @@ typedef struct {
 #endif
 
 /**
+ * @brief   Pad event enable.
+ * @details This function programs an event callback in the specified mode.
+ * @note    The default implementation not necessarily optimal. Low level
+ *          drivers may  optimize the function by using specific hardware
+ *          or coding.
+ * @note    Programming an unknown or unsupported mode is silently ignored.
+ * @note    The function can be called from any context.
+ *
+ * @param[in] port      port identifier
+ * @param[in] pad       pad number within the port
+ * @param[in] mode      pad event mode
+ * @param[in] callback  event callback function
+ *
+ * @special
+ */
+#if !defined(pal_lld_enablepadevent) || defined(__DOXYGEN__)
+#define palPadEnableEvent(port, pad, mode, callback)
+#else
+#define palPadEnableEvent(port, pad, mode, callback)                        \
+  pal_lld_enablepadevent(port, pad, mode, callback)
+#endif
+
+/**
+ * @brief   Pad event disable.
+ * @details This function disables previously programmed event callbacks.
+ * @note    The default implementation not necessarily optimal. Low level
+ *          drivers may  optimize the function by using specific hardware
+ *          or coding.
+ * @note    The function can be called from any context.
+ *
+ * @param[in] port      port identifier
+ * @param[in] pad       pad number within the port
+ *
+ * @special
+ */
+#if !defined(pal_lld_disablepadevent) || defined(__DOXYGEN__)
+#define palPadDisableEvent(port, pad)
+#else
+#define palPadDisableEvent(port, pad)                                       \
+  pal_lld_disablepadevent(port, pad)
+#endif
+
+/**
  * @brief   Reads an input line logic state.
  * @note    The function can be called from any context.
  *
@@ -619,6 +676,40 @@ typedef struct {
 #else
 #define palSetLineMode(line, mode) pal_lld_setlinemode(line, mode)
 #endif
+
+/**
+ * @brief   Line event enable.
+ * @note    The function can be called from any context.
+ *
+ * @param[in] line      line identifier
+ * @param[in] mode      line event mode
+ * @param[in] callback  event callback function
+ *
+ * @special
+ */
+#if !defined(pal_lld_lineenableevent) || defined(__DOXYGEN__)
+#define palLineEnableEvent(line, mode, callback)                            \
+  palPadEnableEvent(PAL_PORT(line), PAL_PAD(line), mode, callback)
+#else
+#define palLineEnableEvent(line, mode, callback)                            \
+  pal_lld_lineenableevent(line, mode, callback)
+#endif
+
+/**
+ * @brief   Line event disable.
+ * @note    The function can be called from any context.
+ *
+ * @param[in] line      line identifier
+ *
+ * @special
+ */
+#if !defined(pal_lld_linedisableevent) || defined(__DOXYGEN__)
+#define palLineDisableEvent(line)                                           \
+  palPadDisableEvent(PAL_PORT(line), PAL_PAD(line))
+#else
+#define palLineDisableEvent(line) pal_lld_linedisableevent(line)
+#endif
+
 /** @} */
 
 /*===========================================================================*/

User avatar
Giovanni
Site Admin
Posts: 12179
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 571 times
Been thanked: 488 times
Contact:

Re: Use of GPIO interrupts in EX drivers

Postby Giovanni » Tue May 31, 2016 8:39 am

Hi,

The safest way is to zip files before attaching them, there are security reasons so the forum does not accept potentially harmful extensions.

The patch is very close to what I was thinking about, the only thing I would change so far is to pass to the callback a "line" rather port and bit. I would also add an "attribute" to the PAL driver PAL_SUPPORTS_CALLBACKS, this way old implementations are compatible.

Merging will have to wait a bit, I am working on other things right now.

Giovanni

awygle
Posts: 32
Joined: Sun Apr 03, 2016 8:39 pm
Has thanked: 6 times
Been thanked: 4 times

Re: Use of GPIO interrupts in EX drivers

Postby awygle » Tue May 31, 2016 7:29 pm

Line is no problem, I'll make that change. It's only in the port LLD so it doesn't change the patch.

No parameter is necessary, the drivers still build and link without it - the EnableEvent and DisableEvent functions are simply stubbed out if there aren't corresponding LLD macros. This is the pattern of all the other macros in the PAL as well. I tested this on several of the STM32 demos.

I wish we could all just get together and decide what extensions are dangerous :D I've had other systems reject .zips for that very reason. A zipped version is attached.

Of course you are a very busy man, we all appreciate your work! I only hope this is a little helpful :)
Attachments
pal_event.zip
(1.19 KiB) Downloaded 67 times


Return to “ChibiOS/EX”

Who is online

Users browsing this forum: No registered users and 1 guest