added receive functionality to the cc26x0 uart #2

dcellucci · 2018-04-27T23:15:19Z

Adding RX Functionality

This is part of a larger push to add a bootloader to the CC26xx series micros. I added RX functionality to the CC26x0 UART (which should be transferable to the CC26x2 micro).

I also needed to update the tock submodule with the latest pull, because it contains the updated console capsule. In order to get it to build, I added the sleep function to chip.rs.

I tested with the test/console app, and it was happy.

brghena · 2018-04-28T20:06:31Z

chips/cc26x0/src/uart.rs

+
+        for i in 0..rx_len {
+            rx_buffer[i] = self.read_byte();


Reading all the bytes here one at a time is likely a bad choice. The entire system will hang until all the requested bytes are read, which could be an indefinite period of time (for instance, forever if there is a mismatch between expected number of bytes and what actually got received, or several seconds if a human is typing data in through a keyboard).

For the SAM4L platform, we implemented a non-blocking receive interface using the DMA. https://github.com/tock/tock/blob/master/chips/sam4l/src/usart.rs#L853 It starts a reception when commanded, but allows it to run in the background and performs the callback whenever all the data is received. Non-blocking code is certainly a lot more effort to write, but it allows the platform to be more robust because the kernel and applications can continue running while waiting on a reception.

Now, if the goal is to support a bootloader, blocking code might be acceptable. But, in my opinion, I don't think it should be exported through the UART HIL for userland code to use.

That totally makes sense. I avoided the use of DMA because I'm having trouble debugging Rust code (it won't let me set breakpoints in gdb). Any advice you'd have there would be appreciated.

I'm also noticing that the power-management-exp branch is like 75 commits ahead of master - should I be developing on that?

Also, probably worth pointing out that the transmit method is blocking if the message is larger than the FIFO buffer. I attempted to set it up with interrupts (similar to the NRF52 UART), but had trouble getting the TX_FIFO_READY interrupt to flag.

@dcellucci sorry for the late answer, it's been a holiday-extended-weekend here in Sweden!

The power-management-exp is mostly experiments to reduce the energy consumption, and most of those 75 commits is experiments; not including many changes. It's easier if you'd continue to develop against master, and we'll merge later on (there won't be any substantial conflicts in the UART implementation apart from pin configurations).

dcellucci · 2018-04-29T03:39:40Z

Okay I've started implementing the DMA code, and I've hit sort of a roadblock:

The DMA block requires a contiguous, 1024-byte block of memory, aligned to the nearest 1024-byte boundary. This block stores source/destination pointers and I'm thinking of implementing this as a static array in main.rs- is there a particularly Rust way of doing this you'd recommend?

brghena · 2018-04-29T03:58:22Z

Making a contiguous block of memory could definitely just be done with a static array in main.rs. But aligning it to the nearest boundary is difficult. You'd have to go into the LD file, add it as a memory section, and align it there. For example, APP_MEMORY is linked to the .app_memory section.

Requiring something like this would definitely make the cc26xx different from the SAM4L and nRF5x chips that we currently support DMA for. After you can get something working, we'll have to think more generally about how to support needs like that in Tock. I'm not very familiar with the DMA on the cc26xx series. What page in the Technical Reference Manual describes the need for this block of memory?

dcellucci · 2018-04-29T04:15:43Z

The section is: 12.3.5 Channel Configuration. It's on page 1155 of my copy of the TRM.

Okay linker it is (I was kind of hoping you wouldn't say that!).

brghena · 2018-04-29T04:25:14Z

I understand. Yup, doing something like the .app_memory section and a static allocation in main.rs is exactly what you want to do.

This interface makes a lot of sense, but it is annoying that it has to be constrained to 1024 Byte boundaries. For now, just make the change directly to the layout.ld file. We'll have to think about ways to add chip or board-specific sections to memory. Maybe with an additional LD file import...

…trol table, in the linker and main.rs and verified their locations in memory. dma_control_table: 0x20002000, app_memory: 0x20002400. App memory actually has more space than before (not sure if the top of the ram was being used)

alevy · 2018-04-29T11:57:34Z

I think you can also use #[repr(align = "N")]" though haven't tried: https://github.com/rust-lang/rfcs/blob/master/text/1358-repr-align.md

dcellucci · 2018-04-29T15:49:03Z

Good lead! I’ll check it out

brghena · 2018-04-29T18:44:17Z

Good call Amit. A start could be something like:

#[repr(align = "1024")]
struct Control_Table {
   config_array: [DMA_Config; 32],
}

struct DMA_Config {
    source_ptr: usize,
    dest_ptr: usize,
    control: usize,
    _unused: usize,
}

Eventually, we'd have to really think about the types of the source and destination pointers, since they're a vulnerability.

…modding the linker

dcellucci · 2018-04-30T03:29:16Z

works! Thanks for the insight! I'll get to work fleshing it out, but it does align the table to the nearest 1024 byte boundary (i'm getting 0x20000400).

On a side note, I'm using the launchpad and not the sensortag as my dev board. It uses different LED/UART pins- any thoughts on a low-footprint way to allow me to keep track of these two different pinouts? Right now I keep separate branches and merge them whenever I push, it's kind of clunky...

brghena · 2018-04-30T03:39:05Z

The easiest option is probably create a new board file for the launchpad. All you'd have to do is copy the entire directory including your changes into a new folder in boards/ and then change the package name in Cargo.toml https://github.com/tock/tock-sensortag/blob/master/boards/sensortag/Cargo.toml#L2 You should make the new folder name and the package name match, and they ought to in some way identify the board you're using (although specific details about which board it is can go in the Readme)

…udma

cpluss

Took a quick look. It will be a hassle to merge this with the power-management-exp PR ( #1 ) due to the formatall. I think it's better to perform a formatall in the master branch and rebase both this PR and #1. It'd also make the files-changed in this branch less and more relevant to the changes made.

cpluss · 2018-05-02T09:29:55Z

boards/sensortag/src/main.rs

@@ -246,6 +233,7 @@ pub unsafe fn reset_handler() {
    let mut chip = cc26x0::chip::Cc26x0::new();

    debug!("Initialization complete. Entering main loop\r");
+    debug!("Location of DMA memory: {:p}",&udma::DMACTRLTAB);


We should probably remove both of these debug!s later on

cpluss · 2018-05-02T09:33:57Z

chips/cc26x0/src/uart.rs

+    pub fn read_byte(&self) -> u8 {
+        // Get byte from RX FIFO
+
+        while !self.rx_ready() {}


The UART implementation can cause problems due to these "blocking-if-not-ready" in both RX and TX if the debugger isn't attached (if you're using the launchpad, you can test by removing the jumpers for UART between the board and the debugger), since it'll be blocking indefinitely.

We should probably add a way to detect whether or not UART is attached, and either not invoke any of these or simply return (eg. if !self.initialized.get() { return })

I'm going to rewrite the whole UART to look similar to the NRF52 interface, once I get DMA working, and I will make sure that this is included!

cpluss · 2018-05-02T09:36:54Z

chips/cc26x0/src/udma.rs

@@ -0,0 +1,56 @@
+///


Awesome that you started on this!

Thanks! I'm going to shoot for an MVP right now, but I think there's a larger conversation about what the DMA manager should look like. It seems like the SAM4L model might be the closest analog...

cpluss · 2018-05-02T09:38:09Z

boards/sensortag/src/main.rs


 #[link_section = ".app_memory"]
-static mut APP_MEMORY: [u8; 10240] = [0; 10240];
+static mut APP_MEMORY: [u8; 11264] = [0; 11264];


Why did you increase the app memory?

oh I had initially partitioned off a section of .app_memory in the linker for the DMA control table, and in doing so had to recalculate the size of that section. I'll change it back :)

dcellucci · 2018-05-02T16:29:24Z

re: the make formatall fiasco: sorry about that I will attempt to revert those changes...

dcellucci · 2018-05-07T04:11:21Z

chips/cc26x0/src/udma.rs

 struct DMAConfig {
    source_ptr: usize,
    dest_ptr: usize,
-    control: usize,
+    control: ReadWrite<u32,DMATableControl::Register>,


Having trouble here- I'd like to implement the control pointer as a register, since it has a register-like structure. Unfortunately, I need to implement copy and clone in order to get this to work in its current form. That means I need to modify the mod.rs file in the kernel (which I don't want to do just for this...)

Hrm... that's a bummer. I don't see any harm in adding Copy and Clone to ReadWrite (since it's really just a wrapper around an IntLike (which already needs to implement Copy and Clone)

dcellucci · 2018-05-07T04:12:59Z

chips/cc26x0/src/udma.rs

        ControlTable {
-            config_array: [0; 1024]
+            config_array: [DMAConfig {


another option is to replace this array initialization with some kind of manual initialization where 32 separate instances of DMAConfig are cast from 4 byte blocks of memory. This also doesn't seem like the right thing to do.

Oh, I see, it only needs to be copyable in order to initialize cleanly? Would a macro do the trick?

Oh yeah, this is a thing in Rust. rust-lang/rfcs#1109

None doesn't implement copy either, so it's why the process array needs to be:

static mut PROCESSES: [Option<&'static mut kernel::Process<'static>>; NUM_PROCS] = [ None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, None, ];

A general macro-based solution here would solve both of the problems. This answer claims to do it (https://stackoverflow.com/questions/28656387/initialize-a-large-fixed-size-array-with-non-copy-types/28666474#28666474) but I'm not sure if it's what we want.

@brghena exactly- I was balking at the mem::uninitialized call (the compiler gave me all sort of red flags)

maybe the process array approach is the best idea- each channel needs to be enabled anyway. I'll try it out and report back!

dcellucci

I'm having trouble implementing an array of ReadWrite registers, basically. what would be the best way to do this? The only way I could get it to compile was by modifying mod.rs to derive clone and copy for the ReadWrite struct. Doesn't seem right...

dcellucci · 2018-05-20T00:26:27Z

Hi all,

Sorry for not working more on this- I've been pretty swamped

I seem to have hit a roadblock- I'm maxing out the RAM available to the CC26x0. It could be because I had to add that 1024-byte block of control vectors for the DMA, but that seems like a pretty razor thin margin to begin with.

If you all have any thoughts I'd appreciate it! Maybe I'm doing something stupid with Rust's reference system that is causing me to make a bunch of copies...

Alternatively I could switch to the CC26x2, which has 4x the RAM, if this just looks like how things are.

Edit: A little update. I modded the linker so it would build and I could see how big everything is.

This is the linker output for master:

       text	   data	    bss	    dec	    hex	filename
  44900	   1780	  16272	  62952	   f5e8	target/thumbv7m-none-eabi/release/sensortag

This is the linker output for launchpad-uart-rx (I cheated and made the RAM region larger):

   text	   data	    bss	    dec	    hex	filename
  46476	   4776	  15876	  67128	  10638	target/thumbv7m-none-eabi/release/sensortag

So we only had like 2 KB of RAM to begin with even before implementing DMA, and we lose another KB of RAM just to the DMA table. Probably makes sense to graduate to the CC26x2.

dcellucci · 2018-05-20T02:09:57Z

I'm reexamining my code and I think I should do some editing. I made some pretty memory-poor choices and will fix and then repush....

…rk in a separate branch lol)

dcellucci · 2018-05-23T05:51:14Z

Okay my problem was (surprise surprise) the changes I made to the .app-memory section . turns out there were good reasons for not making it bigger...

Anyway, working DMA w/ UART, tested using the console example. Maybe not ready for merge but ready for feedback.

cpluss

Awesome that you got it working, nice job! :)

I added some review comments regarding the code - more nit comments rather than actual implementation feedback

cpluss · 2018-05-23T08:03:37Z

chips/cc26x0/src/uart.rs

        }
    }

+    pub fn configure_dma(&self) {
+        unsafe{


unsafe { // Do unsafe stuff }

To make the unsafe block more explicit use indentation. No need for two separate unsafe by the way, imo it would be better to make the entire function configure_dma as unsafe instead

cpluss · 2018-05-23T08:06:08Z

chips/cc26x0/src/uart.rs

+
+        self.disable_interrupts();
+
+        if unsafe{udma::UDMA.transfer_complete(TX_DMA)} {


I'd rather have the unsafe block outside of an if-conditional, such as

let transfer_complete = unsafe { ... }; if transfer_complete {

Otherwise the conditional becomes kind of messy

cpluss · 2018-05-23T08:09:30Z

chips/cc26x0/src/uart.rs

+        //initialize the UART with the passed settings, and then transfer them
+        //to the DMA controller
+        //is there a particular reason we use tx_len rather than truncated_len?
+        //(as with the NRF52, etc.)


tx_data.len() will return the allocated size of the slice (e.g. [0; 32] will return 32), which is why we use tx_len as we don't want to transmit the entire buffer every time :)

assuming tx_len is correctly set then tx_len <= tx_data.len() should hold

cpluss · 2018-05-23T08:11:02Z

chips/cc26x0/src/udma.rs

+        &self,
+        dma_channel: DMAPeripheral, 
+        width: DMAWidth, 
+        xfer_type: DMATransferType,


Please use more descriptive variable names if it's not too long, transfer_type is more explicit and clear than xfer_type

cpluss · 2018-05-23T08:11:16Z

chips/cc26x0/src/udma.rs

+    }
+
+
+    pub fn start_xfer(


start_transfer

cpluss · 2018-05-23T08:11:25Z

chips/cc26x0/src/udma.rs

+        registers.set_channel_en.set(1<<(dma_channel as u32));
+    }
+
+    pub fn prepare_xfer(


prepare_transfer

cpluss · 2018-05-23T08:12:16Z

chips/cc26x0/src/udma.rs

+
+    }
+
+    pub fn do_xfer(


do_transfer, or prepare_and_start_transfer would be even more explicit; I get the impression from do that the transfer will be complete once the function returns

honestly, I borrowed it from the SAM4L DMA interface, as with the xfer vs. transfer naming conventions. I don't actually use the method, and it might be worth deleting it (I prefer being more explicit when it comes to DMA)

Inspired by tock/tock-sensortag#2 (comment) We can afford the four extra characters.

dcellucci · 2018-05-29T05:48:53Z

I just merged in the new master branch, but the RX part of the console app is no longer working (TX still works). It looks like there were quite a few changes to the power management part of the chip def. I'll be happy to debug but I think someone with an eye for the new power management system could see what's wrong much faster.

Also is it just me or is anyone else not able to drop breakpoints in using gdb?

cpluss · 2018-05-29T07:23:58Z

Deep sleep is probably interfering with the interrupts (we don't wake up on all interrupts in deep sleep), change this to return Sleep and it should work :)

lowest_sleep_mode should return DeepSleep when it is safe to transition into deep sleep (i.e. the receive functionality is not needed)

cpluss · 2018-05-29T07:32:32Z

Another solution would be to add a new wakeup event, see p. 248 of the TRM. It's easy enough to test by modifying L157 of aon.rs :)

Should probably rewrite and refactor the aon module to allow changes to the wakeup sources to occur dynamically

dcellucci · 2018-05-29T15:51:03Z

Thanks alot for the reply!

The link you attached drops me at the top of the diff- where exactly should I put that Sleep return?
EDIT: I looked at the ble.rs file as an example and I figure you mean the lowest_sleep_mode function. I'm going to do a check if self.rx_remaining_bytes == 0 and decide whether DeepSleep or Sleep is the correct choice from that.

Also it doesn't appear that UART RX is a wakeup event. I'm thinking I should attach it to an Edge Detect on the RX pin instead, though that seems like it would work better when there's a dynamic interface (so we can set it based on the UART settings)

dcellucci · 2018-05-29T16:49:30Z

Adding that check appears to produce a working console app!

:D thanks for all your help everyone!!!

dcellucci · 2018-05-30T19:40:52Z

I think it's ready for review!

cpluss

Nice! There seems to be some commented code in the dma implementation still, is there a reason to keep this?

cpluss · 2018-05-31T11:16:11Z

chips/cc26x0/src/uart.rs

+        }
+
+        let rx_transfer_complete = unsafe{udma::UDMA.transfer_complete(RX_DMA)};
+


Can rx_transfer_complete and tx_transfer_complete both be true?

I think so- they're separate channels in the DMA controller so they can both be running, and while they point to the same register (UART DR), one is purely reading the other writing.

cpluss · 2018-05-31T11:16:49Z

chips/cc26x0/src/uart.rs

@@ -214,25 +388,50 @@ impl kernel::hil::uart::UART for UART {

        self.disable_interrupts();
        self.set_params(params);
-        self.configure();
+        self.configure(); 


Remove this space?

cpluss · 2018-05-31T11:17:17Z

chips/cc26x0/src/uart.rs

    }

    fn transmit(&self, tx_data: &'static mut [u8], tx_len: usize) {
-        if tx_len == 0 {
+        let truncated_len = min(tx_data.len(), tx_len);


Is this really necessary, why not use tx_len and trust that the arguments are correct?

I think it's worth including in this case. We tend to do it in other similar places in the kernel.

In general it doesn't hurt to be defensive since the compiler will add bounds checks (which will panic if out of bounds) and will slide them if it can tell the bounds have been checked explicitly.

cpluss · 2018-05-31T11:18:15Z

chips/cc26x0/src/uart.rs

+            chip::SleepMode::DeepSleep as u32
+        } else {
+            chip::SleepMode::Sleep as u32
+        }


How does it work if the MCU goes into deep sleep, can it still receive and wake up?

Not right now. I tried to implement a mod to aon.rs and I couldn't get that mod to work. For the interim I'm just going to leave it at Sleep (which can receive) until we figure out a way to get waking from deep sleep working.

cpluss · 2018-06-06T15:30:04Z

chips/cc26x0/src/uart.rs

-            chip::SleepMode::Sleep as u32
-        }
+    fn lowest_sleep_mode(&self) -> u32 {      
+        chip::SleepMode::Sleep as u32


Why did you change this back? This way, it'll never be able to enter deep sleep since UART is registered as a peripheral

Daniel Cellucci added 4 commits April 25, 2018 22:10

working RX (using console app)

42e568f

ran make formatall

ee65277

added receive functionality to the cc26x0 uart

ff28980

ran make formatall

f337df4

brghena reviewed Apr 28, 2018

View reviewed changes

Daniel Cellucci added 2 commits April 28, 2018 22:58

added statically allocated DMA control table section

bce1788

Daniel Cellucci added 3 commits April 29, 2018 20:22

working aligned DMA CTRL Table without using custom linker

d2da82f

Merge branch 'launchpad-uart-rx' into cc26x0-uart-rx

49021c4

cleaned up a few comments, working aligned DMA control table without …

7ce18c7

…modding the linker

working configuration (right now using a private static array inside …

1d978a2

…udma

cpluss reviewed May 2, 2018

View reviewed changes

Daniel Cellucci added 2 commits May 6, 2018 21:07

tried to implement register changes

6d1a4e3

Merge branch 'launchpad-uart-rx' into cc26x0-uart-rx

d5cd118

dcellucci commented May 7, 2018

View reviewed changes

Daniel Cellucci added 4 commits May 8, 2018 08:20

static block initialization, nothing fancy

72a1d75

Merge branch 'launchpad-uart-rx' into cc26x0-uart-rx

8ba03bd

successfully builds

a58c54d

removed copy clone derive from core, still builds

8d0d23f

Daniel Cellucci added 5 commits May 20, 2018 13:45

ready for testing

dfadaf3

working TX

30ac41d

added the buffer vector to the right spot

1d45bf0

working transmit/receive (finally)

523fbaf

changed main.rs to be compatible with sensortag pinouts (forgot to wo…

469b0d1

…rk in a separate branch lol)

cpluss requested changes May 23, 2018

View reviewed changes

ppannuto added a commit to tock/tock that referenced this pull request May 24, 2018

sam4l: rename xfer to transfer

d48a3a8

Inspired by tock/tock-sensortag#2 (comment) We can afford the four extra characters.

ppannuto mentioned this pull request May 24, 2018

sam4l: rename xfer to transfer tock/tock#953

Merged

3 tasks

Daniel Cellucci added 4 commits May 23, 2018 20:15

made requested changes

cb97e27

merged updated master

252d7dd

UART tx works but not rx. maybe it's not waking up from sleep?

202d8e9

Merge branch 'launchpad-uart-rx' into cc26x0-uart-rx

603cbc9

changing sleep settings

91e1975

Daniel Cellucci and others added 5 commits May 29, 2018 09:52

removed extraneous comments

836b78a

added launchpad board definition

51ad227

added code for io.rs

7eeede2

merged in latest master updates

78f9e75

addressed compiler warnings

e2af3c0

cpluss requested changes May 31, 2018

View reviewed changes

removed commented code

1a5081b

cpluss requested changes Jun 6, 2018

View reviewed changes


		self.disable_interrupts();

		if unsafe{udma::UDMA.transfer_complete(TX_DMA)} {

		}

		let rx_transfer_complete = unsafe{udma::UDMA.transfer_complete(RX_DMA)};

added receive functionality to the cc26x0 uart #2

Are you sure you want to change the base?

added receive functionality to the cc26x0 uart #2

Conversation

dcellucci commented Apr 27, 2018

Adding RX Functionality

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcellucci Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcellucci commented Apr 29, 2018

brghena commented Apr 29, 2018

dcellucci commented Apr 29, 2018

brghena commented Apr 29, 2018

alevy commented Apr 29, 2018

dcellucci commented Apr 29, 2018

brghena commented Apr 29, 2018

dcellucci commented Apr 30, 2018

brghena commented Apr 30, 2018

cpluss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcellucci commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcellucci left a comment

Choose a reason for hiding this comment

dcellucci commented May 20, 2018 • edited Loading

dcellucci commented May 20, 2018

dcellucci commented May 23, 2018

cpluss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcellucci commented May 29, 2018

cpluss commented May 29, 2018

cpluss commented May 29, 2018

dcellucci commented May 29, 2018 • edited Loading

dcellucci commented May 29, 2018

dcellucci commented May 30, 2018

cpluss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcellucci Apr 29, 2018 •

edited

Loading

dcellucci commented May 20, 2018 •

edited

Loading

dcellucci commented May 29, 2018 •

edited

Loading