Remove the need for explicit critical sections in interrupts

[FlyingRatBull]

Right now, interrupt handlers are fn() and if you want to modify anything inside a Mutex you need to do:

::avr_device::interrupt::free(|cs| {
    // Use cs here
});

However after reading a bit more into the ATmega328p Datasheet regarding general functionality of interrupts on AVR microprocessors, I stumbled upon the fact, that interrupts are disabled inside an interrupt handler (unless explicitly enabled by using nested interrupts):

When an interrupt occurs, the global interrupt enable I-bit is cleared and all interrupts are disabled. The user software can write logic one to the I-bit to enable nested interrupts. All enabled interrupts can then interrupt the current interrupt routine. The I-bit is automatically set when a return from interrupt instruction – RETI – is executed.

Thus using ::avr_device::interrupt::free inside an interrupt handler only benefits one to get the CriticalSection instance, not for the actual enabling/disabling of interrupts.

Therefore I propose to change the signature of interrupt handlers to fn(CriticalSection). This would improve the crate in a few ways:

• More precisely reflect the actual situation on an AVR microprocessor in that interrupt handlers are already run inside a critical section
• Remove the need for the user to call unsafe code to get a CriticalSection instance: let cs = unsafe { CriticalSection::new() };
• Remove the need for the user to call ::avr_device::interrupt::free to get a CriticalSection instance
• Reduce binary size by 10 bytes per interrupt handler that previously had to use ::avr_device::interrupt::free

[Rahix]

Yeah, this was also discussed a bit in #52 back then. I am in favor of this change and I don't see any problems.

My only comment would be that the cs argument to the interrupt handler should be optional — this keeps things backward-compatible and doesn't clutter code of users who don't need the critical section.

[FlyingRatBull]

While I agree regarding backwards-compatibility, I tend to have a slightly different opinion regarding clutter:

If a user has a fn ISR() and needs a CriticalSection (e.g. for a Mutex), the thought process would likely be something like:

• I have a Mutex
• To access it, I need a CriticalSection
• (look up CriticalSection) I get a CriticalSection by calling ::avr_device::interrupt::free()

Thus resulting in unneeded added code/complexity.

Therefore I personally think it should be non-optional with a deprecation-period with a warning for the fn() type. Alternatively the fact, that you can use fn(CriticalSection) instead would have to be very well documented to avoid this scenario.

[Rahix]

Therefore I personally think it should be non-optional with a deprecation-period with a warning for the fn() type.

I am not entirely happy with this, but I see your point so let's give this approach a try. :)