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

Added DMA control for UART messages #83

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Added DMA control for UART messages #83

wants to merge 13 commits into from

Conversation

slanab
Copy link
Contributor

@slanab slanab commented Aug 9, 2018

Pull request addressing issue #36
Attached sample test program.
obc_sci_dma_test.txt

@slanab slanab requested a review from richarthurs August 9, 2018 04:37
@richarthurs
Copy link
Contributor

Sorry it has taken me so long to get to looking at this, today has been the first day I could do sat stuff for the past week.

Tried out the test and it works. I think the task now is to replace the current serial sends with the DMA enabled versions. sfu_tasks.c/vSerialTask() implements the task where strings are pulled off the RTOS transmit queue and sent out (line 76). sfu_uart.c/serialsendln() and serialSendln() can be changed to make the DMA calls too.

@richarthurs
Copy link
Contributor

Looking at the technical reference manual (pg. 545)

The DMA controller cannot recognize two software requests on the same channel if the first
software request is still pending. If such request occur DMA will discard it. Therefore the user
software should check the pending register before issuing a new software request.

I am guessing that waiting for the transfer complete interrupt to fire would cover this, but that should be confirmed.

@liquiddandruff
Copy link
Member

Very nice! As for considering replacing all SCI calls with the DMA version though, it may be the case that small transfers might still be quicker than having them done via DMA. Something to be tested and verified.

@richarthurs
Copy link
Contributor

richarthurs commented Aug 15, 2018

I also think that the serialDMASend should wait for the DMA transfer interrupt to fire before returning which should stop this scenario:

  • task has a buffer, writes some stuff into it
  • task calls serialSendQ with that buffer
  • serial task unblocks, serialSendQ dequeues the pointer to the buffer, starts initiating a DMA transfer
  • < DMA transfer is happening >
  • serialDMASend returns, now we're back in the task
  • task reuses that buffer < DMA transfer is still happening >
  • now the data that DMA has transferred to the UART is garbled since we changed the source before DMA request could complete

Assuming my understanding of the whole thing is correct, this case should be rare/almost impossible since DMA transfer should be pretty fast. But I still think we should protect against it happening. It'll be a short wait compared to sending the bytes out individually, so still gets us the efficiency gain from using DMA.

A couple of other things:

  • Steven's comment brings up a good point. Tech reference manual (TRM) doesn't say much about DMA transfer time with different frame sizes (string lengths). The execution time could be measured with the PMU
  • I like the idea of having DMA and non-DMA serialSend functions
  • We have a lot of ways to send stuff to UART, there should be a mutex applied to all UART send functions.
    • Except possibly for the functions that use DMA. I think the UART hardware should arbitrate between an in-progress DMA transfer and incoming bytes from firmware (though the TRM should be checked to see if this is right), so no need to mutex that.

@richarthurs
Copy link
Contributor

richarthurs commented Aug 15, 2018

Ok so I tried this out and my wait for interrupt to fire point is something we need to watch for.

If you do this:

	/* Setup null terminated string to transmit */
	number_string((char *) buffer, 500);
    scidmaSend(buffer);
	buffer[300] = 'A';
    scidmaSend(buffer);

You end up with an "A" coming out the first UART transfer, meaning the buffer was updated before the DMA got around to piping byte 300 to the UART. This is a bit contrived since the buffer change should be atomic, but it shows the problem.

@richarthurs
Copy link
Contributor

Nice! This will be useful!

  • include sfu_hardwaredefs.h and replace sciREG with UART_PORT so that it’s portable between chips. I added this to the dev guide: https://github.com/SFUSatClub/obc-firmware/wiki/ORCAASAT-Dev-Guide so it's actually documented for the future.

  • looks like some of the commented out things in uart_dma_test() can be removed (I think you should leave number_string though since it’s a good demo)

  • uart_dma_test() can be moved down to the tests section of that file (sorry, I was the one who put it in the wrong place originally)

  • call uart_dma_init() at the top of task_main instead of in the test so that we can disable the test call.

  • remove char buffer[4 * 500]; from the header and put in the .c file (just in the test function) since we definitely have other variables called buffer and don't want them to get confused.

  • One of the other variables in the header is unused, and the others are global should be implemented according to the Global Variables section of the dev guide: https://github.com/SFUSatClub/obc-firmware/wiki/ORCAASAT-Dev-Guide

Also what was the message rewrite note that's in there? I don't recall what we discussed about that yesterday.

@slanab
Copy link
Contributor Author

slanab commented Sep 12, 2018

It looks like it takes almost the same time to transmit a message using DMA and using regular SCI function so we could just replace the SCI Send function with a DMA one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants