Interrupt api

[bkozak-scanimetrics]

This is a simple, platform independent API for creating critical sections by disabling interrupts globally.

I have also provided a reference implementation in CC26XX and have modified the CC26XX radio driver to use the interface (for the purpose of demonstration and testing).

Eventually we would ideally see implementations for every major platform.

I believe that it is very important that Contiki have such an API so that it is possible to write atomic sections into some of the core, platform agnostic drivers. This will allow developers to take better advantage of interrupt routines in platform agnostic code. It seems to me that in core Contiki use of interrupts is often avoided where they might otherwise be extremely useful (specifically in the ContikiMAC driver) and I intend to start changing this.

[vsaw]

Loads of code inside contiki already uses the established spl methods.

Have you considered using this instead of proposing your own version of SPL?

[tonnenpinguin]

What happens if you enter into the atomic section inside one scope (say in a switch statement) and exit it in another scope? Wouldn't you have a non declared variable then?
Despite that some older compilers probably won't accept variable declarations in the middle of the code...

[bkozak-scanimetrics]

@tonnenpinguin

ATOMIC_SECTION_ENTER starts a block scope which ATOMIC_SECTION_EXIT ends. This will do two things:

1. You will not be able to use ATOMIC_SECTION_ENTER in a different scope than ATOMIC_SECTION_EXIT (the compiler will complain about an unmatched curly brace)
2. Even old compilers will allow the variable definition since the variable is defined at the start of a new scope

With that said, this still has its problems but I've chosen to implement it this way because this is already a fairly common idiom IMO (see for example http://www.keil.com/forum/20720/atomic-sections-of-code/)

[tonnenpinguin]

@bkozak-scanimetrics

ah true, you're right!
I personally would prefer the ATOMIC_SECTION approach over the spl one (I have never seen that being used either), but I think we potentially impose weird behaviour if we solve it using unmatched curly braces (think of a atomic section ended in an if statement).

In the link you posted they use a global variable to save the interrupt status. We could use this and declare a global variable (extern uint8_t/bool _ints_disabled or something) in isrControl.h and define it in some .c file (then we would need a isrControlArch.c though)

[bkozak-scanimetrics]

@tonnenpinguin

Regarding point 1 above, I've made a mistake. There will most likely be a compiler error of some sort but it may not be regarding an unmatched brace. This is, unfortunately one of the weaknesses of this implementation.

[bkozak-scanimetrics]

@tonnenpinguin

Honestly I agree. The compiler errors produced by this approach can indeed be very surprising and I really don't like that.

Regarding the use of a global variable in this way, the problem with that approach is that it does not account for the possibility that ATOMIC_SECTIONs might be nested (for example if a function within an atomic section tries to create its own atomic section).

The only way I know of that both allows nested sections and avoids the weirdness with my current macros is to use a global counter (lock) variable which is incremented for each interrupt disable and decremented for each enable.

This is the approach used in the CMSIS library for energy micro (see INT_Disable and INT_Enable in https://code.google.com/p/rt-thread/source/browse/trunk/bsp/efm32/Libraries/efm32lib/inc/efm32_int.h?r=1525). The only reason I don't like this approach is because of the possibility of integer overflow (but maybe it is reasonable to just assume that this will never happen in any real software).

[tonnenpinguin]

@bkozak-scanimetrics
I agree that nesting atomic sections is not possible with a global variable.

Regarding integer overflow: if you manage to get a uint32 to overflow I suppose using your proposed technique you would have gotten a stack overflow way earlier ;-)

Despite that I feel somewhat uncomfortable with the function not turning on interrupts when the counter already was zero I agree that that would be a good option!

One question though: why are you using int's instead of int8_t / int16_t's?
That's probably another one of my personal issues but I tend to use fixed size integers as it is a real pain to find errors related to different integer sizes on different systems

[bkozak-scanimetrics]

@tonnenpinguin

You're quite right regarding the overflow; I suppose I'm mostly just being puritanical. I also agree about the behaviour of INT_Enable with a lock count of zero but I'm really not sure what a better behaviour would be.

I had chosen to use an int initially because I originally just needed a boolean variable (and I suppose I have a habit of using int in place of bool). If I make the change to use a lock counter (which I am thinking is probably what I'll do if there are no further objections) I'll be sure to use a uint32_t to avoid overflows anyway and platforms that need to store the priority information in a global can do so in whatever word size is appropriate.

[bkozak-scanimetrics]

@vsaw

Thanks for your input!

I am perfectly happy to use splhigh / splx as long as we can make this a core interface that every platform will (eventually) implement. My main concern is to make it so that platform independent code can disable interrupts (i.e. the code that lives under core, app examples).

I'd never seen SPL before you mentioned it and, would have never expected that splhigh() / splx would have anything to do with setting interrupt priority. I think these are bad function names but if you think that most developers would recognize this for what it is then maybe you are right and we should have an SPL implementation for all platforms (I'd love to hear some more opinions on this).

In spite of the naming, I can see why it would be necessary on some platforms to have the function that restores the interrupt priority take as an argument the value returned by the function that sets the interrupt priority. I hadn't though about this before because I am doing my development work on ARM where there are instructions that disable and enable interrupts globally (i.e. without touching priority registers). I will make this change.

[bkozak-scanimetrics]

As per my discussion with tonnenpinguin, I'm now using a global lock variable to make atomic sections safe and easy.

I'd like to point out that I've created a new C file just to hold the lock variable. I don't particularly like this but I couldn't see any other obvious place to put it. If anyone has a better idea of where it should go I'm all ears.

[tonnenpinguin]

Probably a personal preference, but I like to include the Header file that requires the local variable definition so I know what it is needed for without having to search for it in the docs!

[bkozak-scanimetrics]

@tonnenpinguin

I had originally included the header file but removed it after finding that it produced a compiler warning (unused function) on arm-none-eabi-gcc-4.8.2. Your comment has prompted me to look into this further, however, and I've now found that this problem is caused by the fact that the CC_INLINE macro is empty on the platform I am using (cc26xx).

In light of this I think I will put the include back in the C file.

[bkozak-scanimetrics]

@vsaw

I've considered putting the function code in the C file but have opted to keep it in the header file because I have reasoned that it might result in a performance increase which makes it worth the increased code size and stylistic concerns. Also, I think that it is particularly important to keep atomic sections as small as possible and that defining these functions in a header as static inline might just help with this ever so slightly.

I am by no means entirely convinced of this, however, and would love to hear arguments one way or the other.

For now, I'll hold off on making this change.

[bkozak-scanimetrics]

Regarding what I think may be a lack of interest in this PR, is it perhaps the case that I need to add some actual uses of my code in core before it can be accepted?

Personally, I would disagree with this approach, but I might add this code as part of a larger commit that actually requires it's use (i.e. something in core that needs interrupts to be disabled).

I understand that it can be difficult to justify adding code that is both mostly unused and requires architecture implementations to be useful which is why I would really appreciate any feedback whatsoever, positive or negative, from anyone with an opinion.

[atiselsts]

My two cents: I like the idea (this feature has long been missing from Contiki), but not the implementation.

I would support explicitly declaring a local variable to hold the interrupt state, as the spl() family functions do. (The name "spl" is bad though.) I don't see justification of hiding these actions in a macros. Particularly the global variable idea feels ugly. One reason is that from my experience with bugs in embedded applications, global memory corruption is more likely than stack memory corruption. Consider that the global variable and therefore the interrupt state can be screwed by any invalid write happening anywhere in the program... and this kind of errors are extremely hard to track.

There is also overhead: the inline atomic_section_enter/exit function will typically add significant code memory overhead, and the global variable has a constant 4 byte RAM overhead.

For the local variable that holds interrupts enabled/disabled state, maybe it would be better to use a platform specific typedef that expands to either uint16_t or uint32_t, instead of using a bool. Then the API later could be generalized to disable/enable different subclasses of interrupts on platforms which support that.

The naming currently mixes upper and lower case characters both for macro/function names and file names. This looks ugly and is not consistent with the rest of Contiki code.

[bkozak-scanimetrics]

Particularly the global variable idea feels ugly.

I quite like the global variable approach for its simplicity of use.

One reason is that from my experience with bugs in embedded applications, global memory corruption is more likely than stack memory corruption.

This is true in my experience also but I don't think that this should be a major consideration here because I don't think its reasonable to try and design around random memory corruption (aside from detection methods and fail-safes like watchdogs).

There is also overhead: the inline atomic_section_enter/exit function will typically add significant code memory overhead, and the global variable has a constant 4 byte RAM overhead.

I'm with you on this though.

For the local variable that holds interrupts enabled/disabled state, maybe it would be better to use a platform specific typedef that expands to either uint16_t or uint32_t, instead of using a bool. Then the API later could be generalized to disable/enable different subclasses of interrupts on platforms which support that.

This is a good idea.

The naming currently mixes upper and lower case characters both for macro/function names and file names. This looks ugly and is not consistent with the rest of Contiki code.

I don't think its ugly but it certainly isn't consistent with Contiki style and now that you've mentioned it I see a few other stylistic issues that need to be fixed. Thanks for that.

I'm thinking that I should just go ahead and turn this into an spl like interface just because it is the most simple and obvious way to do this. Although I like the lock counter approach it's fancier than what is absolutely required and could always coexist with the simpler interface and be added in later if it's something that people like.

I'll start work on this and if anyone wants to voice their preference to keep the global lock counter please do so.

Thanks for your input @atiselsts

[kkrentz]

It would be very nice to have an API for atomic sections. However, I have a question regarding your implementation: what happens with interrupts that are issued while being in an atomic section? I am afraid that interrupts that issue while being in an atomic section are simply ignored, causing other code that waits for certain interrupts (e.g. rtimer) to wait forever. Or is it like every pending interrupt is executed afterwards?

[bkozak-scanimetrics]

Or is it like every pending interrupt is executed afterwards?

Yup, that's how it works.

With my implementation, interrupts that occur within an atomic section are moved into the 'pending' state. When the atomic section ends the service routine fires which will clear the pending bit.

Every CPU I've ever worked with has this feature.

Depending on the source of the interrupt, it is possible that events may be swallowed only if more than one of the same kind of interrupt occurs within the atomic section (this isn't usually even a problem however since it's often possible to handle all of the events from the same ISR).

So, in summary, you won't lose any interrupts so long as your atomic sections are 'short enough'.

[kkrentz]

Good to know.

Looking at isr-control.h, I would actually like to get rid off the parameter state of the function atomic_section_exit. Doing so would free us from declaring a local state variable every time we use this API. Also, this will decrease code size. What do you think about this way of implementing isr-control-arch.h:

static int counter;

static CC_INLINE void
atomic_section_enter(void)
{
  counter++;
  ti_lib_int_master_disable();
}

static CC_INLINE void
atomic_section_exit(void)
{
  if(!--counter) {
    ti_lib_int_master_enable();
  }
}

[bkozak-scanimetrics]

If you scroll up to this comment you'll see that I already proposed this but some contributors don't like the global counter approach.

Personally, I'm fine with that approach but it really is debatable which way is better.