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.

Sign up for GitHub

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

Jump to bottom

drivers: nrf: Fix nrf uarte fifo_fill function. #11475

Merged
merged 2 commits into from
Nov 22, 2018
Merged

drivers: nrf: Fix nrf uarte fifo_fill function. #11475

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
356f734
drivers: nrf: Fix nrf uarte fifo_fill function.
Nov 19, 2018
507678a
drivers: nrf: nrf uarte fix TX interrupts disabling.
Nov 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions drivers/serial/uart_nrfx_uarte.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ struct uarte_nrfx_data {
uart_irq_callback_user_data_t cb; /**< Callback function pointer */
void *cb_data; /**< Callback function arg */
u8_t *tx_buffer;
volatile bool disable_tx_irq;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the type should be a atomic_t type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only independent load and store operations are used which are itself atomic, there is no need for atomic type.

#endif /* UARTE_INTERRUPT_DRIVEN */
};

Expand Down Expand Up @@ -82,7 +83,22 @@ static inline NRF_UARTE_Type *get_uarte_instance(struct device *dev)
static void uarte_nrfx_isr(void *arg)
{
struct device *dev = arg;
const struct uarte_nrfx_data *data = get_dev_data(dev);
struct uarte_nrfx_data *data = get_dev_data(dev);
NRF_UARTE_Type *uarte = get_uarte_instance(dev);

if (data->disable_tx_irq &&
nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX)) {
nrf_uarte_int_disable(uarte, NRF_UARTE_INT_ENDTX_MASK);

/* If there is nothing to send, driver will save an energy
* when TX is stopped.
*/
nrf_uarte_task_trigger(uarte, NRF_UARTE_TASK_STOPTX);

data->disable_tx_irq = false;

return;
}

if (data->cb) {
data->cb(data->cb_data);
Expand Down Expand Up @@ -275,6 +291,9 @@ static int uarte_nrfx_fifo_fill(struct device *dev,
struct uarte_nrfx_data *data = get_dev_data(dev);
const struct uarte_nrfx_config *config = get_dev_config(dev);

if (!nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX)) {
return 0;
}

if (len > config->tx_buff_size) {
len = config->tx_buff_size;
Expand Down Expand Up @@ -320,29 +339,32 @@ static int uarte_nrfx_fifo_read(struct device *dev,
static void uarte_nrfx_irq_tx_enable(struct device *dev)
{
NRF_UARTE_Type *uarte = get_uarte_instance(dev);
struct uarte_nrfx_data *data = get_dev_data(dev);

nrf_uarte_int_enable(uarte, NRF_UARTE_INT_ENDTX_MASK);
data->disable_tx_irq = false;
}

/** Interrupt driven transfer disabling function */
static void uarte_nrfx_irq_tx_disable(struct device *dev)
{
NRF_UARTE_Type *uarte = get_uarte_instance(dev);

nrf_uarte_int_disable(uarte, NRF_UARTE_INT_ENDTX_MASK);

/* If there is nothing to send, driver will save an energy
* when TX is stopped.
*/
nrf_uarte_task_trigger(uarte, NRF_UARTE_TASK_STOPTX);
struct uarte_nrfx_data *data = get_dev_data(dev);
/* TX IRQ will be disabled after current transmission is finished */
data->disable_tx_irq = true;
Copy link
Member

Choose a reason for hiding this comment

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

You should also clear this flag in uarte_nrfx_irq_tx_enable.

}

/** Interrupt driven transfer ready function */
static int uarte_nrfx_irq_tx_ready_complete(struct device *dev)
{
NRF_UARTE_Type *uarte = get_uarte_instance(dev);

return nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX);
/* ENDTX flag is always on so that ISR is called when we enable TX IRQ.
* Because of that we have to explicitly check if ENDTX interrupt is
* enabled, otherwise this function would always return true no matter
* what would be the source of interrupt.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't get it. This function is supposed to return true exactly when the NRF_UARTE_EVENT_ENDTX event is set. And it is not true that this event is always set. It is cleared before a transmission is started, see uarte_nrfx_fifo_fill.

Copy link
Collaborator

@lemrey lemrey Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

For the application it is a problem if
after disabling interrupts uart_irq_tx_ready() returns true.

Copy link
Member

Choose a reason for hiding this comment

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

For the application it is a problem if
after disabling interrupts uart_irq_tx_ready() returns true.

Then perhaps the application should be corrected.
The UART API does not describe such behavior as required or expected, does it?

Copy link
Contributor

@jakub-uC jakub-uC Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Application assumes correctly that it is allowed to send data with uarte_nrfx_fifo_fill and ignore tx interrupt by disabling it. I am pretty sure this is not problem of an application.

When application is used with UART (not UARTE) this works as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

The UART API does not describe such behavior as required or expected, does it?

It does. If it doesn't that's bug. Indeed, uart_irq_tx_ready() should be gated on on interrupts being enabled. Well, I don't know if there're some apps which can be fooled otherwise, but we can't dismiss that. The issue, there's single ISR for both UART RX and TX (and for errors, about which a few people remember). So, ISR can be called by RX interrupt, and should not be wrongly doing TX actions if TX is effectively disabled (and if TX interrupt is disabled, then effectively TX is disabled).

Copy link
Member

@anangl anangl Nov 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I created an issue (#11535) for clarification of this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR fixes current issues with nrf uarte driver acting differently than nrf uart driver. I agree that API has to be clarified and it should be discussed, but changes in this PR don't affect it and are needed now. Also there is ongoing work on new API #10820 which will eventually make current interrupt driven API obsolete and we should focus on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm +1 on this logic, let me approve.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Let's go on with this fix for now.

*/
return nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX) &&
nrf_uarte_int_enable_check(uarte, NRF_UARTE_INT_ENDTX_MASK);
}

static int uarte_nrfx_irq_rx_ready(struct device *dev)
Expand Down