Add RMT receiver #104
Merged
Add RMT receiver #104
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
How do I run the ci.yml on my fork on my local pc? |
https://github.com/nektos/act could work, alternatively, you can keep pushing here and we'll squash the commit history when its ready to merge :). |
added 3 commits
July 22, 2022 09:55
Changed line 835 to carrier_rm
rmt.rs - changed line 835 to carrier_rm
usbalbin
reviewed
Jul 25, 2022
usbalbin
reviewed
Jul 25, 2022
|
When testing rmt-transceiver.rs I see the following output. In theory the transmit time for the RMT signal (total time) should be 3.930 milliseconds. The Tx loop from above has timestamp 378, the 2nd and 3rd RX loop time stamps are 868 and 1368. Not sure why one of those Rx loop timestamps did not pick up the 1st Tx loop? The 2nd Tx Loop and beyond gets picked up by the Rx loop. Strange? |
|
Maybe the problem (or maybe it is not a problem) is with logging. In the screen capture below it shows 2 Rx Loops one right after another (timestamp 2408, 2948) but only one Tx Loop (timestamp 2378). From ten on out I see one Tx Loop and 2 Rx Loops (one with data the other empty) which is correct since Rx Loop is every 500 milliseconds and Tx Loop is every 1 second. |
|
Can someone explain what the Clippy errors mean. |
CI was stuck on an old nightly which broke libc, its now fixed with this commit in master 5edffda. If you rebase this branch on top of master it should fix the errors :). |
|
I am getting build errors that I do not understand why they are happening. These occur in the example file rmt_transceiver.rs Is there some reason we can't use log in examples? |
Too long to explain... :( please use |
Change gpio pins for esp32-c3
|
Would it be possible to rebase this PR for latest |
|
It looks like a lot of changes to rmt.rs |
By all means keep Do you think you could rebase your changes on top of latest master? The previous release I've kept in a separate branch, but I don't have the time to push into it new functionality. It is only there for bugfixes that would fix crashes and other gross errors. |
|
@enelson1001 The PR is still reported as having conflicts by github. |
|
I have been playing some more with the RMT Receiver and I think I would like to change PulsePair duration0 and duration1 to u16 types. I need the duration value from PulslePair not PulseTicks ( I cannot get the 0 field of PulseTicks because it is private). In my example I need to compare duration0 and duration1 so the duration's are between a minimum value and a maximum value. My partial example Is this the correct way to go or is there a better way? |
added 2 commits
November 9, 2022 08:35
- use u16 instead of PulseTicks
ivmarkov
requested changes
Nov 10, 2022
There was a problem hiding this comment.
I think this driver is in a great shape! I have several suggestions which achieve one thing - getting rid of the requirement for "alloc" and simultaneously - making the API somewhat compatible with e.g. how the Read Rust trait is modeled as well as how other "data receiving" drivers in the HAL work.
Once you address the changes, we should merge. Just in time for the new release of the HAL from master.
| /// Use [`RxRmtDriver::start()`] to receive pulses. | ||
| /// | ||
| /// See the [rmt module][crate::rmt] for more information. | ||
| use alloc::vec::Vec; |
There was a problem hiding this comment.
I don't think this driver needs alloc at all - see below.
| pub struct RxRmtDriver<'d> { | ||
| channel: u8, | ||
| _p: PhantomData<&'d mut ()>, | ||
| pub pulse_pair_vec: Vec<PulsePair>, |
There was a problem hiding this comment.
Why should that be a member at all? Remove. (And see below for an alternative suggestion.)
| _channel: impl Peripheral<P = C> + 'd, | ||
| pin: impl Peripheral<P = impl InputPin> + 'd, | ||
| config: &ReceiveConfig, | ||
| rx_buffer_size_in_bytes: u32, |
There was a problem hiding this comment.
This should be usize, not u32. I also suggest to change the semantics of this to be size-in-PulsePair-instances, i.e. rename to just rx_buffer_size and do the math to bytes inside the constructor.
| }, | ||
| }; | ||
|
|
||
| let pulse_pair_size: u32 = rx_buffer_size_in_bytes / 4; |
There was a problem hiding this comment.
Do the opposite math here.
| }; | ||
|
|
||
| let pulse_pair_size: u32 = rx_buffer_size_in_bytes / 4; | ||
| let pulse_pair_vec: Vec<PulsePair> = |
|
|
||
| unsafe { | ||
| esp!(rmt_config(&sys_config))?; | ||
| esp!(rmt_driver_install(C::channel(), rx_buffer_size_in_bytes, 0))?; |
There was a problem hiding this comment.
Opposite math and cast as u32.
| } | ||
|
|
||
| // Set ticks_to_wait to 0 for non-blocking. | ||
| pub fn get_rmt_items(&mut self, ticks_to_wait: u32) -> Result<u32, EspError> { |
There was a problem hiding this comment.
- Rename to simply
receive ticks_to_waitshould be ofTickType_ttype. See also thedelaymodule. This way the user can e.g. passdelay::BLOCKhere.- Method should take a
&mut [PulsePair]buffer reference - Method should return
usize, notu32, which would be the number of items fetched from the ring buffer and written back to the user-supplied buffer
| esp!(rmt_get_ringbuf_handle(self.channel(), &mut rmt_handle)) | ||
| .expect("Failed to get ringbuffer handle"); | ||
|
|
||
| let rmt_items = xRingbufferReceive(rmt_handle as *mut _, &mut length, ticks_to_wait) |
There was a problem hiding this comment.
Change this to xRingbufferReceiveUpTo and pass to it <user-supplied-buf-reference>.len() * 4
| } | ||
|
|
||
| impl PulsePair { | ||
| pub fn new(lvl0: u32, dur0: u32, lvl1: u32, dur1: u32) -> Self { |
There was a problem hiding this comment.
Given that this constructor takes "raw" ESP IDF values of type u32 I don't think it should be public.
|
I am testing latest master (I think). In my project Cargo.toml i have the following. But when I try and compile I get the following error (I am not using SPI at the moment) First question: Second question: |
|
And thanks for finishing the RMT receive. It would have taken me another month, probably more to implement the changes you made. I still have a lot to learn. |
Yes.
No. You are seeing this because your toolchain is too old. With that said, I'll remove the turbofish when calling |
I would appreciate if you test the changed code though. I don't have a setup for testing the RMT stuff. |
|
Tested example but it is crashing. I changed CONFIG_ESP_MAIN_TASK_STACK_SIZE=14000 but that did not help. Question 1 Question 2 But get the following error. |
|
The cause of the crash is this line. I put a println statement after this line of code and it never gets printed. |
You are changing the stack of the main task/thread, while the buffer is allocated on another task/thread. I've fixed the example accordingly.
That's an omission. I've added a |
|
Example fails with different type of failure. I added a couple of println. Seems to fail in TX thread. |
|
Looking at rmt.rs and what changed. In esp-idf-v4.4 file components/esp_ringbuf.c Where before it used Could this be the problem? |
Yes, that's the problem. The ring buffer in the RMT driver is NOSPLIT and unfortunately is not a byte buffered one, so the The problem with the regular Not everything is lost though. What we can do - in case the user supplied buffer is not large enough - is to preserve - in a pair of temporary (raw pointer, length) state variable - the data which is in the ring buffer and which was received and then return an error to the user that he has to pass a bigger buffer. Once we get the bigger buffer, we can fill it in with the data preserved in the temporary state and only then return it (i.e. mark it as free) to the ring buffer. |
|
When I used the ringbuffer in the past esp-idf would spit out an error message that the ringbuffer had overflowed. But can't remember how it handled the buffer after the overflow notification. |
|
If the RMT interrupt code cannot insert data in the ringbuffer, I think this data would simply be dropped to the floor. And this is I think orthogonal to our discussion anyway. |
|
... as we are discussing what to do when reading from the ring buffer, not what happens when the interrupt code of the driver pushes stuff to the ring buffer. And reading - one way or another - has to be quick anyway, yes. Or else you'll have data dropped. But we have no control over that. It is up to the user. |
Is there a limit on how big this buffer can grow? I guess it depends upon how big the stack is in the thread? |
|
Nobody says that the user should provide a buffer allocated on the stack. |
|
@enelson1001 OK so I've quickly prototyped a possible solution. Not sure it will work right out of the box, but you can give it a try! |
|
yes |
|
|
Sorry forgot to do cargo update. trying again |
|
Looks good |
|
Maybe I should try with buffer set to 4? To see how it reacts. |
|
Buffer set to 4. Program output |
|
Sorry but I am back to this comment again. But to provide the bigger buffer won't the user have to do the following.
From my point of view I think its better not to add the functionality to fix the users error of providing the wrong size of the buffer and let esp-idf report the buffer overflow at runtime. It seems to me we are trying to fix the user problem - if user increases buffer to some set max size and it still not big enough we will still have an error. I am not sure how easy it would be to create a new RmtRxDriver (getting peripherals) if program is outside main but I am not an expert in rust programming. Anyway my thoughts |
|
I'm talking about the |
|
Maybe we are talking apples and oranges. I assumed the user would set the pulses buffer size the same as ringbuffer size in RxRmtDriver. Maybe you are assuming the user would set ringbuffer size in RxRmtDriver to X and then user set pulses buffer size to less than X. If the user sets ringbuffer size to the same size as pulses buffer size and the pulses buffer is not big enough then he just can't change pulses buffer size to fix. He also needs to change RxRmtDriver ringbuffer size. I tried this on the rmt example. Set the ringbuffer size to 4 in the RxRmtDriver (will not hold response) Set pulses buffer size to 20 which is more than enough to hold response. Say user started with pulses buffer size at 4 and then changed buffer size to 20 in program Ran the program. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding receive capability to RMT. I am new to rust so code should be reviewed as it might not represent the rust way of doing things.