While reading a book about modern C++, I have seen a code snippet that confused me. The code has been written to set up PWM (16 bit Timer) for 8 bit AVR microcontroller. The code is like that:
class pwm_base : private util::noncopyable
{
public:
typedef std::uint_fast16_t duty_type;
pwm_base(const duty_type resol,
const duty_type duty = 0U) : resolution(resol),
counter (0U),
duty_cycle(duty),
shadow (duty) { }
duty_type get_resolution() const { return resolution; }
void set_duty(const duty_type duty)
{
// Set a new duty cycle in the shadow register.
mcal::irq::disable_all();
shadow = static_cast<std::uint_fast8_t>((std::min)(duty, get_resolution())); // ???? (1)
mcal::irq::enable_all();
}
duty_type get_duty() const
{
// Retrieve the duty cycle.
mcal::irq::disable_all();
const volatile std::uint_fast8_t the_duty = duty_cycle; // ???? (2)
mcal::irq::enable_all();
return the_duty;
}
virtual void service() = 0;
protected:
const duty_type resolution;
duty_type counter;
duty_type duty_cycle;
duty_type shadow;
};
I have problems with lines indicated by ????. It is clearly seen that both, shadow and duty_cycle have been defined as uint_fast16_t. So, it means that they have to be at least 16 bits. If it is the case, why does the author downcast the result of min method to uint_fast8_t instead of casting to uint_fast16_t at line ???? (1) ? And also, why does at line ???? (2) he downcast again the variable to uint_fast8_t even if the function return type is uint_fast16_t? Are these downcastings required? What are their purposes?
This code seems severely over-engineered, which is always the biggest danger whenever you allow C++. The language tends to encourage making things needlessly complicated just for the heck of it, instead of utilizing the KISS principle.
The hard requirements are: on the given hardware, the CPU works fastest with 8 bit types but the PWM register is 16 bits. Period.
Therefore any variable describing period or duty cycle needs to be exactly 16 bits. It doesn't make sense to declare it as uint_fast16_t because the corresponding hardware registers will always be exactly 16 bits, no more, no less.
In general the uint_fast types are only helpful if planning to port this to a 32 bit system somehow, but it is very unlikely that there will exist a 32 bit MCU with an identical PWM peripheral. Thus in this case uint_fast is just useless noise, because the code will never get ported.
The cast to uint8_t supposedly truncates the 16 bit value to the 8 least significant bits. If so, the cast is incorrect, it should never be static_cast<std::uint_fast8_t> but static_cast<std::uint8_t>. I'm otherwise not quite following what the code is supposed to do, update the duty cycle somewhere, I would assume.
Furthermore, disabling the interrupt mask as a re-entrancy protection feature probably doesn't make sense, as this can cause unexpected timing problems. If this class is to be used from inside an ISR then another protection mechanism might be more suitable, such as a semaphore or atomic access/inline asm. PWM in particular is picky with when/how you change the duty cycle, or you might get glitches.
Overall, I would strongly recommend not to use C++ on an ancient, severely resource-confined 8 bit microcontroller. Or to use ancient 8 bit microcontrollers instead of ARM when learning embedded systems, 8-bitters are much harder to program in C (or C++) than 32-bitters.