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

Bipropellant protocol #72

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

p-h-a-i-l
Copy link

Work in progress.
already tested and working: USART3 PWM control over bipropellant protocol

@EFeru
Copy link
Owner

EFeru commented Jun 27, 2020

Awesome that you are working on it! I still have to look a bit in more detail over the code.

At a quick look, I have one remark. Instead of interrupt based UART can we use the DMA UART that I made, with circular buffer and Idle Line interrupt? You can check the framework in util.c
It works like a charm on my hovercar, not a single timeout. So, it would be a pity not to use it. Plus it also gives consistency in the firmware.

@p-h-a-i-l
Copy link
Author

It is a work in progress, not everything implemented and some parts do nothing which need to be removed.
I'd rather use DMA based UART for sure, but unfortunately I don't know how to get messages with variable length. Do you have an idea?
Sending should be rather simple, for receiving I need something which feeds protocol_byte() with received bytes to process

@EFeru
Copy link
Owner

EFeru commented Jun 27, 2020

Yes, I did that already for the Serial Debug. If you look at this function in util.c
https://github.com/EmanuelFeru/hoverboard-firmware-hack-FOC/blob/d8b529e063b1a03908cd1508e05af5b329eb348b/Src/util.c#L818-L841
And below Is the processing done byte by byte. The len of the received data can vary.
https://github.com/EmanuelFeru/hoverboard-firmware-hack-FOC/blob/d8b529e063b1a03908cd1508e05af5b329eb348b/Src/util.c#L957-L970

Of course, you can create you own custom usart_process_data(...) according to your needs. What processing do you need to do?

@p-h-a-i-l
Copy link
Author

very nice! Changed the implementation now to DMA USART.

@EFeru
Copy link
Owner

EFeru commented Jun 28, 2020

2 more remarks :)

  • What is the scope of SERIAL_USART2_DMA and SERIAL_USART3_DMA? Can we give a more suggestive name? maybe... PROTOCOL_SERIAL_USART2 and PROTOCOL_SERIAL_USART3, depends for what they are used for. And for consistency, in general is nice if we keep ..._USART2 , ..._USART3 at the end of the name, to match the rest of the Serial defines.
  • and I have seen in some places (maybe you removed them already) pragma once. Apparently, Keil is more restrictive and doesn't like it. So, can we replace them with:
// Define to prevent recursive inclusion
#ifndef zzz_H
#define zzz_H
.....
#endif // zzz_H

@p-h-a-i-l
Copy link
Author

Two easy points to change. Btw, you should also be able to commit to my branch if you like to contribute.
Right now I think the use of ifdefs is a bit excessive, I'd like to clean up a little, but it is not a priority.

@p-h-a-i-l
Copy link
Author

@EmanuelFeru: Can you have a look at my usage of the sending routine? I guess I'm doing something wrong. Receiving is working well, but sending is very unstable..

I guess the problem is in USART3_DMA_send()..

You are using:
if(__HAL_DMA_GET_COUNTER(huart3.hdmatx) == 0) {
HAL_UART_Transmit_DMA(&huart3, (uint8_t *)uart_buf, strLength);
}
What is are you doing with the Counter? Do I need to implement a buffer?

@EFeru
Copy link
Owner

EFeru commented Jun 29, 2020

The check below has to be there to make sure the DMA counter (Counts down) has reached 0. This indicates that previous data has been sent.

if(__HAL_DMA_GET_COUNTER(huart3.hdmatx) == 0)

Now, if you try to send data faster than it can be physically sent, then not all your data will be send, because the if check will not pass all the time. In this case buffering is needed, to put all data in a buffer and load the DMA once. But if you make sure to have some time in between the sends, then buffering is not needed.
Try to see how it behaves with the if condition and check that you are not calling the send function one after each other (without any time in between).

Are you sending ASCII data? I am not sure if it will make a difference, but I compute the length using strlen((char *)(uintptr_t)data) and it works. It might work also with sizeof(data), but I haven't played with it.

@p-h-a-i-l
Copy link
Author

Something is very fishy.. Indeed __HAL_DMA_GET_COUNTER is not ready yet. But when I wait for it to clear, way too much time goes by.
Also looks like some Messages are sent too short.

@p-h-a-i-l
Copy link
Author

Ok, I guess I know whats happening.. I need a buffer. Just sending garbage from reused memory locations

@p-h-a-i-l
Copy link
Author

Ok, with a buffer the garbage problem is solved. Still the delay is huge.
I have a test setup running a simple ping command. Usually I can see a ping of 14ms (protocol relayed via Wifi to UART) when using interrupt based UART, witth DMA it is around 500ms to 700ms

@EFeru
Copy link
Owner

EFeru commented Jun 30, 2020

Hmm, that is strange, because I do send data to my Sideboard every ~10 ms depending on the main loop duration (see below). If I have time I will quickly check with an oscilloscope if data is indeed sent at ~10ms intervals.
https://github.com/EmanuelFeru/hoverboard-firmware-hack-FOC/blob/d8b529e063b1a03908cd1508e05af5b329eb348b/Src/main.c#L413-L443

@EFeru
Copy link
Owner

EFeru commented Jul 1, 2020

Apparently, I created a conflict in util.cwith my last commit, but I don't see the conflict... do you know how to fix it?

EFeru added 2 commits July 2, 2020 12:14
- replaced `enable` with `m_motorsEnable`
- fix renamig of `timeout` to `timeoutCnt`
@EFeru
Copy link
Owner

EFeru commented Jan 1, 2022

Hi @p-h-a-i-l , First of all happy new year!
What is you plan with this PR, intend to continue on it?

@Rubiaceae65
Copy link

Hi, is this pr working in the current state? Or is there still some things that need to be fixed?

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