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

fix(PeriphDrivers): Fix UART DMA Transactions and Enable Full Duplex #763

Merged
merged 28 commits into from
Nov 30, 2023

Conversation

Jake-Carter
Copy link
Contributor

@Jake-Carter Jake-Carter commented Oct 10, 2023

Description

Our UART DMA transactions were essentially unusable, and only happened to work if the transaction function happened to acquire DMA channel 0.

This PR fixes #761

It implements 2 features:

  1. UART DMA "Auto handlers"
  2. APIs for using application-defined DMA handlers and channels

DMA Auto Handlers

This behavior is disabled by default... but I would like to make this the default. Please comment.

Users now have the option to enable "auto handlers" for the UART DMA transactions with

MXC_UART_SetAutoDMAHandlers(MXC_UART_GET_UART(...), true);

When auto handlers are enabled, the drivers will automatically acquire a DMA channel, enable the IRQ, assign an internal handler function, then release the acquired channel when the transaction's finished. This makes UART DMA simpler to set up and use.

New APIs for Assigning DMA Channels

int MXC_UART_SetTXDMAChannel(mxc_uart_regs_t *uart, unsigned int channel);

int MXC_UART_GetTXDMAChannel(mxc_uart_regs_t *uart);

int MXC_UART_SetRXDMAChannel(mxc_uart_regs_t *uart, unsigned int channel);

int MXC_UART_GetRXDMAChannel(mxc_uart_regs_t *uart);

New APIs have been added for managing the UART DMA channels manually. The application code is responsible for acquiring a channel and assigning an appropriate handler. The application is also responsible for releasing the channel when necessary.

void UART_DMA_Handler(void)
{
    MXC_DMA_Handler();
}

int main()
{
    /* This section shows the setup required for using application-defined 
    UART DMA handlers.  The application code is responsible for acquiring 
    DMA channels, assigning the right IRQ to an ISR, calling the default
    DMA handler, and releasing acquired channels if necessary. */
    error = MXC_DMA_Init(DMA_INSTANCE);
    if (error) {
        printf("Failed to initialize DMA with error %i\n", error);
    }

    int channel = MXC_DMA_AcquireChannel(DMA_INSTANCE);
    if (channel < 0) {
        printf("Failed to acquire DMA Channel with error %i\n", error);
    }

    // Enable the IRQ for the acquired channel,
    // and set the ISR to the application-defined handler.
    IRQn_Type irq = MXC_DMA_CH_GET_IRQ(channel);
    NVIC_EnableIRQ(irq);
    MXC_NVIC_SetVector(irq, UART_DMA_Handler);

    // Tell the UART DMA transaction functions to use the acquired channel.
    MXC_UART_SetTXDMAChannel(MXC_UART_GET_UART(CONSOLE_UART), channel);

    // ...
}

 

Checklist Before Requesting Review

  • PR Title follows correct guidelines.
  • Description of changes and all other relevant information.
  • (Optional) Link any related GitHub issues using a keyword
  • (Optional) Provide info on any relevant functional testing/validation. For API changes or significant features, this is not optional.

@github-actions github-actions bot added the MAX32665 Related to the MAX32665 (ME14) label Oct 10, 2023
@Jake-Carter
Copy link
Contributor Author

For readability I've applied these changes for the ME14. Will port after review. The Rev B and C UART drivers may also need these fixes.

@github-actions github-actions bot added the MAX78000 Related to the MAX78000 (AI85) label Oct 20, 2023
@Jake-Carter Jake-Carter added the WIP work in progress label Oct 20, 2023
@github-actions github-actions bot added MAX32520 Related to the MAX32520 (ES17) MAX32570 Related to the MAX32570 (ME13) MAX32650 Related to the MAX32650 (ME10) MAX32655 Related to the MAX32655 (ME17) MAX32660 Related to the MAX32660 (ME11) MAX32670 Related to the MAX32670 (ME15) MAX32672 Related to the MAX32672 (ME21) MAX32675 Related to the MAX32675 (ME16) MAX32680 Related to the MAX32680 (ME20) MAX32690 Related to the MAX32690 (ME18) MAX78002 Related to the MAX78002 (AI87) MAX32662 Related to the MAX32662 (ME12) labels Oct 20, 2023
@Jake-Carter Jake-Carter changed the title fix(PeriphDrivers): Fix UART RevA DMA Transactions fix(PeriphDrivers): Fix UART DMA Transactions Oct 20, 2023
@EricB-ADI
Copy link
Contributor

/clang-format-run

Copy link
Contributor

@Jacob-Scheiffler Jacob-Scheiffler 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 overall. Just a couple of minor comments we should discuss.

@@ -565,11 +622,24 @@ int MXC_UART_RevA_ReadRXFIFODMA(mxc_uart_reva_regs_t *uart, mxc_dma_regs_t *dma,
return E_NULL_PTR;
}

if (states[uart_num].auto_dma_handlers) {
/* Acquire channel */
#if TARGET_NUM == 32665
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to avoid part specific code in the _revX.c files. Can this condition be based on MXC_DMA_INSTANCES instead?

int MXC_UART_RevA_SetAutoDMAHandlers(mxc_uart_reva_regs_t *uart, bool enable)
{
int n = MXC_UART_GET_IDX((mxc_uart_regs_t *)uart);
if (n < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

These checks could be asserts. They are not really time critical, though, so I'll leave the final decision to you.

@Jake-Carter
Copy link
Contributor Author

Thanks @lorne-maxim, updated in 831c893

int MXC_UART_RevA_SetTXDMAChannel(mxc_uart_reva_regs_t *uart, unsigned int channel)
{
int n = MXC_UART_GET_IDX((mxc_uart_regs_t *)uart);
if (n < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is another place where ASSERT may be better. I see other places (not just in the code this PR touches) that could be ASSERTs also. IMO, anything that checks to see if a valid UART instance was passed in is better as an ASSERT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lorne-maxim I ended up purging the asserts except for once in the Init functions. The compiler should catch those cases as type-cast mismatch, so I think the checks for a valid UART instance in every function were overkill anyways...

It reduces our kid size pretty significantly, which is nice

@Jake-Carter Jake-Carter added the API Change This issue or pull request involves a change to the current MSDK API label Nov 30, 2023
@Jake-Carter Jake-Carter changed the title fix(PeriphDrivers): Fix UART DMA Transactions fix(PeriphDrivers): Fix UART DMA Transactions and Enable Full Duplex Nov 30, 2023
@Jake-Carter
Copy link
Contributor Author

Added API change tag for non-breaking additions of new functions

@Jake-Carter Jake-Carter merged commit 16ff45b into main Nov 30, 2023
13 checks passed
@Jake-Carter Jake-Carter deleted the fix/gh-761 branch November 30, 2023 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change This issue or pull request involves a change to the current MSDK API MAX32520 Related to the MAX32520 (ES17) MAX32570 Related to the MAX32570 (ME13) MAX32650 Related to the MAX32650 (ME10) MAX32655 Related to the MAX32655 (ME17) MAX32660 Related to the MAX32660 (ME11) MAX32662 Related to the MAX32662 (ME12) MAX32665 Related to the MAX32665 (ME14) MAX32670 Related to the MAX32670 (ME15) MAX32672 Related to the MAX32672 (ME21) MAX32675 Related to the MAX32675 (ME16) MAX32680 Related to the MAX32680 (ME20) MAX32690 Related to the MAX32690 (ME18) MAX78000 Related to the MAX78000 (AI85) MAX78002 Related to the MAX78002 (AI87)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UART Leaks DMA Channels, Fails After 8 DMA Transactions
5 participants