Empty destructors code of HW drivers

[nvlsianpu]

Description of defect

Most of driver has empty destructor code. The consequence is that there is no possible to de-initialize corresponding hardware in uC. It might caused malfunction when uC common hw part (e.g. GPIO) were used for few peripherial or so. It also might cause incresed power consumption because there is no entry to disable Hardware.

For example on nRF5 target we can use a pin as AnalogIn an after that as DigitalInOut. Once a pin was configured as analog pin, such a pin was assigned to the ADC and the Digital part was disabled. Because of lack of destructor there is no entry to de-initialize analog part of a pin HW or disabling ADC at all. This cause that this pin will be unable to work well if we try to reuse it for DigitalInOut.

example of use case:
https://github.com/ARMmbed/ci-test-shield/blob/master/TESTS/API/AnalogIn/AnalogIn.cpp

Target(s) affected by this defect ?

all

Toolchain(s) (name and version) displaying this defect ?

Not relevant

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.15

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

Not relevant

How is this defect reproduced ?

See the dtor for drivers

[BlackstoneEngineering]

Would you be interested in writing a test that could test for this and adding it to the CI Test Shield Project?

@sg- @screamerbg

[Mirrored to Jira]

[nvlsianpu]

cc @pan-, @anangl

[Mirrored to Jira]

[bcostm]

Also here #2017

[Mirrored to Jira]

[pan-]

Thanks for raising this issue, lack of destructors is present in many driver classes while on others the destructor declared doesn't release any resources. Adding proper release process also means that copy construction and copy assignment should be properly handled, either by forbidding such operations or by reference counting.

This is the list of the classes impacted by these issues:

	Destructor	Copy/Assignment strategy	Note
AnalogIn	✖	✖
AnalogOut	✖	✖
BusInOut	✔	✖	Copy assignment operator declared twice (one public and one private), the public one doesn't make a copy at all and is actually a shorthand for write.
CAN	✔	✖
DigitalIn	✖	✖
DigitalInOut	✖	✖
DigitalOut	✖	✖
Ethernet	✔	✖
I2C	✖	✖
I2CSlave	✖	✖
InterruptIn	✖	✖
LowPowerTicker	✔	✖
LowPowerTimeout	✔	✖
LowPowerTimer	✖	✖
PortIn	✖	✖
PortInOut	✖	✖	Invalid copy assignment operator used for write instead of copy.
PortOut	✖	✖	Invalid copy assignment operator used for write instead of copy.
PwmOut	✖	✖	Invalid copy assignment operator used for write instead of copy.
RawSerial	✖	✖
Serial	✖	✖
SerialBase	✖	✖
SPI	✖	✖
SPISlave	✖	✖
Ticker	✖	✖	Destructor just remove the registered function; it should shutdown the timer if the timer is not used anywhere else.
Timeout	✖	✖	Same as Ticker.
Timer.h	✖	✖
TimerEvent.h	✖	✖	Destructor just remove the registered function; it should shutdown the timer if the timer is not used anywhere else.

[Mirrored to Jira]

[seppestas]

As mentioned in #3945 this issue also affects the use of the improved mbed sleep API.

The LPC176X target doesn't do anything in it's pwmout_free implementation, but e.g Silicon Lab's EFM32 targets requires e.g the pwmout_free function to be called to unlock low power sleep modes when the sleep function is used. This means the low power sleep will not work when a PwmOut object has been initialized, even when it has been deconstructed.

Personally I'm more a fan of some kind of platform independant "disable" function to disable the PWM clock and enable the low power sleep modes again. The same goes for other drivers / peripherals. Currently constantly destructing PWM objects before sleeping and reconstructing them on wakeup seems to be "the way to do it". Not the biggest fan of this approach though, and I have yet to figure out if this causes memory fragmentation issues.
[Mirrored to Jira]

[seppestas]

Would PR's solving parts of this issue get accepted? Maybe it's a good idea to start a feature branch for this?

Low power operation is a must for my use-case (and I imagine many others) and being able to disable hardware is important to accomplish this. This issue is currently the biggest problem for most our projects using mbed, requiring custom patches to mbed and thus greatly increasing complexity (e.g relying on mbed forks, making it harder to do things like update the mbed version).
[Mirrored to Jira]

[0xc0170]

Would PR's solving parts of this issue get accepted? Maybe it's a good idea to start a feature branch for this?

Yes, they could. Please provide tests with any patches. You can also create a new issue or just PR with a description of the problem and a proposal to fix (=solving those parts one by one).

We can create a feature branch if needed.
[Mirrored to Jira]

[seppestas]

Why did #2116 (the oldest still open PR of mbed-os by the way) not get merged?
[Mirrored to Jira]

[0xc0170]

Why did #2116 (the oldest still open PR of mbed-os by the way) not get merged?

As stated there (at least partially). We need to work on the documentation (specifications, what _free should do ?) and tests - to comply with the specifications, and fix all targets. As you noticed some targets have free empty or might not even be implemented.
[Mirrored to Jira]

[seppestas]

So, any updates on this?

The ADC of the nRF53832 SoC (TARGET_NRF5) seems to consume quite a lot of power when it's not being used because of this. It's pretty much impossible to fix this without breaking mbed's HAL.
[Mirrored to Jira]

[0xc0170]

Not at the moment, we are looking at HAL, so will get better specification (also regarding init/free).

You can for now as you noted, add free function call and make it work within nrf5 family.

[Mirrored to Jira]

[seppestas]

I'm bumping this issue again. Not being able to free pins, disable clocks and re-enable sleep modes is a real pain when trying to develop low-power and/or dynamic applications.

We are slowly gathering project/target specific patches to circumvent this issue that we are not able to share since there is still no specification on disabling / freeing drivers. This makes it harder for us to update our mbed project and has a negative effect on the quality of mbed in general.
[Mirrored to Jira]

[ciarmcom]

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers.
Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2132

[ciarmcom]

We closed this issue because it has been inactive for quite some time and we believe it to be low priority. If you think that the priority should be higher, then please reopen with your justification for increasing the priority.

[0xc0170]

I am reopining this as it's tracking issue that is still valid.