Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UARTSerial writes even if tx is disabled #14498

Merged
merged 5 commits into from
Jun 9, 2021

Conversation

ghseb
Copy link

@ghseb ghseb commented Apr 1, 2021

UARTSerial::_tx_enabled and UARTSerial::_rx_enabled shadow SerialBase::_tx_enabled and SerialBase::_rx_enabled.

bool _tx_enabled;
bool _rx_enabled;

bool _rx_enabled = true;
bool _tx_enabled = true;

This essentially leads to data being written out even if the serial is disabled, because UARTSerial::_tx_enabled is always true. In my opinion this is not the desired behaviour.

core_util_critical_section_enter();
if (_tx_enabled && !_tx_irq_enabled) {
UARTSerial::tx_irq(); // only write to hardware in one place
if (!_txbuf.empty()) {
enable_tx_irq();
}
}
core_util_critical_section_exit();

Summary of changes

Remove shadowing variables.

Impact of changes

Migration actions required

Documentation


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


- The variables shadow SerialBase::_tx_enabled and SerialBase::_rx_enabled
@ciarmcom
Copy link
Member

ciarmcom commented Apr 1, 2021

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

@ciarmcom ciarmcom requested a review from a team April 1, 2021 11:00
0xc0170
0xc0170 previously approved these changes Apr 6, 2021
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of why it would shadow it like this.

@mergify mergify bot added needs: CI and removed needs: review labels Apr 6, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 9, 2021

CI restarted

@ghseb
Copy link
Author

ghseb commented Apr 19, 2021

@0xc0170 I think im not able to determine why the check failed.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 19, 2021

True, I'll restart now

@mbed-ci
Copy link

mbed-ci commented Apr 19, 2021

Test run: SUCCESS

Summary: 10 of 10 test jobs passed
Build number : 1
Build artifacts

@ghseb
Copy link
Author

ghseb commented Apr 28, 2021

There probably is a flaw in the UARTSerial/SerialBase implementation hat can cause infinite congestion with this change in place. I'm going to investigate the issue and come back when I have something. I'd propose to not yet merge this one.

@ghseb
Copy link
Author

ghseb commented Apr 28, 2021

The following snippet forces the issue:

    mbed::UARTSerial serial(USBTX, USBRX, MBED_CONF_PLATFORM_STDIO_BAUD_RATE);
    serial.set_blocking(false);
    serial.enable_input(false);
    serial.enable_output(false);

    // Must write at least MBED_CONF_DRIVERS_UART_SERIAL_TXBUF_SIZE bytes to reproduce
    // the issue.
    auto const *fill_string { "Fill UARTSerial buffer" };
    for (int i=0; i<MBED_CONF_DRIVERS_UART_SERIAL_TXBUF_SIZE; ++i) {
        serial.write(fill_string, strlen(fill_string));
    }

    serial.enable_input(true);
    serial.enable_output(true);
    serial.set_blocking(true);

    /**
     * Situation:
     * - tx-buffer of UARTSerial is full
     * - tx-irq is not enabled (`UARTSerial::enable_output(true)` does not enable the tx-irq)
     * - UARTSerial is set blocking
     *
     * If in this situation another write is issued, UARTSerial blocks infinitely because
     * the tx-irq that is supposed to empty the tx-buffer is not active.
     */
    auto const *doom_string { "DOOOOM" };
    serial.write(doom_string, strlen(doom_string));

    // Will never be reached
    auto foo { 2 };

I'd add a fix in this PR.

@mergify mergify bot dismissed 0xc0170’s stale review May 4, 2021 05:56

Pull request has been modified.

Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks basically reasonable to me - just bikeshedding. Going to need this on master too I guess? Is there already another PR for that?

drivers/source/UARTSerial.cpp Show resolved Hide resolved
drivers/source/UARTSerial.cpp Outdated Show resolved Hide resolved
Sebastian Stockhammer added 3 commits May 4, 2021 09:41
- Indicate that UARTSerial::update_rx_irq/update_tx_irq is not symmetric to UARTSerial::disable_rx_irq/disable_tx_irq
- The corresponding section is called from multiple locations now
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. But we need a version for BufferedSerial on master too. Can't be making fixes only on the 5.15 branch.

@ghseb
Copy link
Author

ghseb commented May 4, 2021

Well I never used mbed-os-6 or BufferedSerial until now. I could create a PR but I think im not able to test it in the near future.

@mergify mergify bot added needs: CI and removed needs: work labels May 4, 2021
@ciarmcom ciarmcom added the stale Stale Pull Request label May 10, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented May 19, 2021

Thjank you ! I'll retart CI

@mbed-ci
Copy link

mbed-ci commented May 19, 2021

Test run: FAILED

Summary: 1 of 10 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_example-test-lts

@mergify mergify bot added needs: work and removed needs: CI labels May 19, 2021
@ciarmcom ciarmcom removed the stale Stale Pull Request label May 19, 2021
@mbed-ci
Copy link

mbed-ci commented May 21, 2021

Test run: FAILED

Summary: 1 of 10 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_example-test-lts

@mergify mergify bot added needs: work and removed needs: CI labels May 21, 2021
@ciarmcom ciarmcom added the stale Stale Pull Request label Jun 2, 2021
@ciarmcom
Copy link
Member

ciarmcom commented Jun 2, 2021

This pull request has automatically been marked as stale because it has had no recent activity. @ghseb, please carry out any necessary work to get the changes merged. Thank you for your contributions.

@ciarmcom ciarmcom added stale Stale Pull Request and removed stale Stale Pull Request labels Jun 4, 2021
@ghseb
Copy link
Author

ghseb commented Jun 7, 2021

I think this error is not related.

@0xc0170 0xc0170 added needs: CI and removed needs: work stale Stale Pull Request labels Jun 7, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Jun 7, 2021

I'll restart CI

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 8, 2021

we got an issue it seems with lts example,, I'll investigate today.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 9, 2021

CI restarted (deprecated example was removed)

@mbed-ci
Copy link

mbed-ci commented Jun 9, 2021

Test run: SUCCESS

Summary: 10 of 10 test jobs passed
Build number : 4
Build artifacts

@0xc0170 0xc0170 merged commit a0b02aa into ARMmbed:mbed-os-5.15 Jun 9, 2021
@mergify mergify bot removed the ready for merge label Jun 9, 2021
@ghseb ghseb deleted the uartserial-shadowing branch June 9, 2021 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants