UARTSerial: Free resources

[ghseb]

Description

If output and input is disabled for UARTSerial the pins should be reconfigured to archive lowest power consumption (e.g. to avoid current drain through TX pin).

With this change the UART interface is switched off if both input and output is disabled. If input or output is enabled again the UART pins are reinitialized.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

[ciarmcom]

@ghseb, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

I like the concept, but this is potentially problematic from backwards compatibility because we've never called serial_free before (I think). I don't know how good HAL implementations are.

We would need a specific Greentea test to exercise this in hardware.

[kjbracey]

I'm also not happy with complexity here for being inside a critical section. If init calls can happen inside enable_input, I'd prefer to have the outer logic that manipulates _rx_enabled and _tx_enabled and calls init/free protected by api_lock instead. Then the critical section is reduced only to the bits that needs it - the serial interrupt enable/disable.

[ghseb]

How could a test look like? As far as I know the only default UART is used by greentea itself.

[kjbracey]

Indeed. I know there is some work being done on HAL testing with special loopback adapter boards, but I don't what the state of serial is with respect to that.

Actually, it's probably very rare to destroy a SerialBase anyway, and the addition of the enable calls is brand-new, so there's probably not a lot of code that would be affected by the serial_free addition.

[kjbracey]

Just to save a little ROM, I think I'd leave the locks out here. They're not needed in the constructor and destructor, and UARTSerial has them dummied anyway.

If you're going to be calling init or deinit you need more sequencing than just the basic lock anyway - imagine if someone just decided to do some more serial stuff after deinit did its unlock!

(Maybe stick a _ in front of the method name to give a clue that they're unlocked methods. Some things use that convention, not sure if the serial stuff does).

Let me know if that doesn't make sense to you - I'm not quite 100% confident.

[kjbracey]

Another thought - with this logic, it may be worth putting a base enable_input/output into SerialBase, or maybe even the whole implementation.

It could do it by enabling/disabling interrupts from the HAL layer with serial_irq_set, rather than having UARTSerial detach from the SerialBase.

That would in turn allow testing of the serial_free by doing a disable on the plain Serial in Greentea.

[ghseb]

Another thought - with this logic, it may be worth putting a base enable_input/output into SerialBase, or maybe even the whole implementation.

It could do it by enabling/disabling interrupts from the HAL layer with serial_irq_set, rather than having UARTSerial detach from the SerialBase.

That would in turn allow testing of the serial_free by doing a disable on the plain Serial in Greentea.

I wanted to keep the change as small as possible. For the use-case of avoiding current drain when UARTSerial is used the change as proposed would be sufficient i think.

Putting the whole or a part of the implementation into SerialBase would be quiet a huge change i think, because UARTSerial needs information if rx or tx is enabled and interrupt enable/disable must stay in UARTSerial anyway i think, because its dependent on the buffer state.

[kjbracey]

Yes, it would be a bigger change, but it would extend the logic nicely. The rx_enabled/tx_disabled would go into the SerialBase, because it would be implementing those calls.

The knowledge of interrupt enable vs buffer level comes in via SerialBase making the attach calls, which is then reflected in the _irq array.

A disable input would turn off the HAL interrupt. An enable input would re-enable the HAL interrupt if the corresponding _irq was non-NULL.

The logic gets more complex in SerialBase, but simpler in UARTSerial, as it doesn't need to care whether rx or tx is enabled any more - it just attaches or deattaches the interrupt based on buffer level, and whether those interrupts happen or not depend on the enable in the serial base.

[ghseb]

I dont know if i got everything you mentioned, but i try to propose another solution next week.

[0xc0170]

@ghseb Any update?

[0xc0170]

Closing in favor of #10924