PWMOut operator=() has non const parameter

[potidannon]

Description

Proper "=" overload shall have a const rhs parameter, since it shall never change during copy. some libraries like std::vector requires this formatting.

Current example case is an LPC1768, where I would like to utilize std::vector push_back call. In order to make it work I need an T operator=(const& T) implementation for my class. Within the implementation PWMOut is copied.

PWMOut class "=" overload does not have a const rhs reference, so the copy is not possible in the above mentioned implementation. See the example below.

class ComponentInterface
{
public:      
    //! operator= overload
    ComponentInterface& operator=(const ComponentInterface& right);
private:
    eComponentId            m_Id;                       ///< Idntification of the current component.
#if DEVICE_PWMOUT
    PwmOut                  m_MotorPin;                 ///< CPU pin to manipulate.
#endif
    double                  m_MinPulseWidth_s;          ///< Minimum pulse width (servo dependent)
    double                  m_MaxPulseWidth_s;          ///< Maximum pulse width (servo dependent)

    double                  m_PulseWidth_s;             ///< Current width to be sent to the servo.
    uint16_t                m_ArmPosition_16;           ///< Arm position measured by a rotation sensor.
    uint16_t                m_Torque_16;                ///< Motor torque measured in 16bit data.
    uint32_t                m_StateSwitches;            ///< Contains state of errors and warnings. Each bit represents one type of state                    
};

//-----------------------------------------------------------------------------
ComponentInterface& ComponentInterface::operator=(const ComponentInterface& right)
{
    m_Id = right.m_Id;
    m_PulseWidth_s = right.m_PulseWidth_s;
    m_MotorPin = right.m_MotorPin;
    m_MinPulseWidth_s = right.m_MinPulseWidth_s;
    m_MaxPulseWidth_s = right.m_MaxPulseWidth_s;
    m_ArmPosition_16 = right.m_ArmPosition_16;
    m_Torque_16 = right.m_Torque_16;
    m_StateSwitches = right.m_StateSwitches;
    return *this;
}

Since I don't know other libraries implementing HAL/pwmout_api, I don't know if this is intentional, but I would like to make an enquiry whether you can fix this by introducing "const" qualifier in the right places. See below a proposed fixed version:

// drivers/PWMOut.h

float read() const
{
        core_util_critical_section_enter();
        float val = pwmout_read(&_pwm);
        core_util_critical_section_exit();
        return val;
}

PwmOut &operator= (const PwmOut &rhs)
{
        // Underlying call is thread safe
        write(rhs.read());
        return *this;
}

// HAL/pwmout_api.h
float pwmout_read(const pwmout_t *obj);

// LPC1768 impl
float pwmout_read(const pwmout_t* obj) {
    float v = (float)(*obj->MR) / (float)(LPC_PWM1->MR0);
    return (v > 1.0f) ? (1.0f) : (v);
}

To compile my code I was using online tools, with default settings for LPC1768. Since I am quite new to mbed, I did not change any settings during setting a new program, so I assume I have the latest mbed-os and the toolchain, I guess is set automatically by Pelion, but correct me if I am wrong, please.

Issue request type

[ ] Question
[ ] Enhancement
[ X] Bug

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-1017

[kjbracey]

In this case, the operator= is not doing what you presumably need, which is to make a clone reference to the same pin.

Most of the C++ driver classes are deliberately non-copyable, as you should not be attempting to access the same HAL object through two different objects - it would sidestep the mutex locking, and potentially open them up to thread-safety problems. And we don't know that it's legal to just copy the underlying pwmout_t - we don't know what the HAL has put in there.

PWMOut and DigitalInOut are unusual in that they do have a copy-assignment operator, but they're not CopyAssignable in the sense that vector needs. The copy operator just reads the pwm/pin value from the source and sets the pwm/pin value on the destination.

If you really need to do this sort of vector operation, and you really need the pin to be part of it, I suggest you set up your PwmOuts externally to your ComponentInterface, and have the ComponentInterface store a reference or pointer to a PwmOut.

[potidannon]

Thank you for the heads up. What you suggested below was my workaround, so I am just going to keep that. Since, it looks like it is intentionally implemented like that, I will close this issue.