Skip to content

[Bug] Insufficient initialization of SCI leads to not all desired UART baudrates being possible to select #25

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

Closed
mukyokyo opened this issue Jul 3, 2023 · 15 comments

Comments

@mukyokyo
Copy link

mukyokyo commented Jul 3, 2023

Since you are using fsp, I would recommend using R_SCI_UART_BaudCalculate to minimize the baudrate error.
Currently, the baudrate is initialized with a large error rate, and the R4 is not able to handle the baudrate that the AVR was able to handle.

@mukyokyo
Copy link
Author

mukyokyo commented Jul 3, 2023

I see you are using R_SCI_UART_BaudCalculate.
I was premature. I'm sorry.

I could not communicate with the device communicating at 1Mbps, so I tried to capture the transmitted data with another serial monitor separately and got bit corrupted data.
I will try to measure the error rate in more detail.

I just noticed that err_rate is supposed to handle 1000 times the value, but I thought that if err_rate=5, it would be considered 0.005%.

@aentinger
Copy link
Contributor

Please let us know what you find, meanwhile I'll close this issue as it's root cause was a misunderstanding 😉

@mukyokyo
Copy link
Author

mukyokyo commented Jul 3, 2023

Is it too early to close?

I tried initializing Serial1 with the following baudrates
set:115.2kbps, actual:115.2kbps
set:230.4kbps, actual:230.4kbps
set:500kbps, actual:250kbps <-?
set:1Mbps, actual:500kbps <-?
set:1.5Mbps, actual:750kbps <-?
set:2Mbps, actual:2Mbps

Please try to evaluate the actual performance of all of you.

thanks

@facchinm facchinm reopened this Jul 3, 2023
@facchinm
Copy link
Member

facchinm commented Jul 3, 2023

Hi @mukyokyo ,
if you change line https://github.com/arduino/ArduinoCore-renesas/blob/main/cores/arduino/Serial.cpp#L275 to 5000 is the situation getting better?

@mukyokyo
Copy link
Author

mukyokyo commented Jul 3, 2023

Hi @mukyokyo , if you change line https://github.com/arduino/ArduinoCore-renesas/blob/main/cores/arduino/Serial.cpp#L275 to 5000 is the situation getting better?

It did not change when we actually set 3000 assuming 3%.
From the results, the actual baudrate will not match the setting baudrate in the 250k..1.5Mbps range.
Since R_SCI_UART_BaudSet is being performed without looking at the result of R_SCI_UART_BaudCalculate, I suspect that an inconvenient value may have been adopted.
I will put in the debug code and see what values are actually set in the registers.
Worst case scenario, I will also try to get a follow-up from someone in Renesas.

@aentinger aentinger changed the title Insufficient initialization of SCI [Bug] Insufficient initialization of SCI leads to not all desired UART baudrates being possible to select Jul 3, 2023
@mukyokyo
Copy link
Author

mukyokyo commented Jul 3, 2023

I will just note what I have learned somewhat.

It is clear from the return value of R_SCI_UART_BaudCalculate that the erroneous handling of the digits in err_rate.
It must be specified in permil.

Second, even if R_SCI_UART_BaudCalculate returns FSP_SUCCESS, it does not appear that R_SCI_UART_BaudSet sets the baudrate to the expected baudrate.

Finally, the previous list of baudrates only describes the behavior at those values.
In fact, it was found experimentally that setting the baudrate to a certain range does not result in an unexpected baudrate, but rather an incorrect baudrate when a specific baudrate is specified.

The results are somewhat troublesome so far.

@mukyokyo
Copy link
Author

mukyokyo commented Jul 3, 2023

Since the only way to determine whether the actual baud rate is correct or not is to suspect the value held in the variable.
I dumped the value calculated by R_SCI_UART_BaudCalculate as follows, and it seems that the actual baud rate is halved when mddr=0.

    static int ind = 0;
    const uint32_t baud_list[]={9600,57600,115200,230400,250000,460800,500000,600000,700000,800000,900000,1000000,1500000,2000000,3000000,4000000,6000000};
    fsp_err_t err;
    const bool bit_mod = true;
    const uint32_t err_rate = 3000;  // Meaning 3%
    baud_setting_t uart_baud;

    if (++ind > (sizeof(baudlist) / sizeof(uint32_t) - 1)) ind = 0;
    err = R_SCI_UART_BaudCalculate(baud_list[ind], bit_mod, err_rate, &uart_baud);
    if(err != FSP_SUCCESS) {
      Serial.println("BaudCalculate error");
    } else {
//      err = R_SCI_UART_BaudSet(&uart_ctrl, (void *) &uart_baud);
      Serial.print(" bgdm=");
      Serial.println(uart_baud.semr_baudrate_bits_b.bgdm);
      Serial.print(" abcs=");
      Serial.println(uart_baud.semr_baudrate_bits_b.abcs);
      Serial.print(" abcse=");
      Serial.println(uart_baud.semr_baudrate_bits_b.abcse);
      Serial.print(" cks=");
      Serial.println(uart_baud.cks);
      Serial.print(" brr=");
      Serial.println(uart_baud.brr);
      Serial.print(" mddr=");
      Serial.println(uart_baud.mddr);
      Serial.print(" semr_baudrate_bits=");
      Serial.println(uart_baud.semr_baudrate_bits);
    }

I tentatively tried "const bool bit_mod = false" and now I can generally communicate with the list of baud rates in the previous source (I haven't looked at the actual error rate).

@mukyokyo
Copy link
Author

mukyokyo commented Jul 4, 2023

The most likely possibility is a bug in R_SCI_UART_BaudCalculate.
I tried a simple program in e2studio on a bench with a different MPU, and the actual baudrate is halved by setting a specific baudrate.

Since the condition has been identified, the fix is a simple matter. However, since we will not be taking care of fsp here, we will pass it to the fsp side.
I raised the issue with my contact at renesas.

Finally, I leave it here to note that the baudrate error rate must be specified in multiples of 1000.

thanks

maidnl pushed a commit that referenced this issue Jul 4, 2023
@mukyokyo
Copy link
Author

mukyokyo commented Jul 4, 2023

I have come up with a cute solution that is not affected by the fsp update and I present it to you. I have actually confirmed that the problem goes away.

  err = R_SCI_UART_BaudCalculate(baudrate, bit_mod, err_rate, &uart_baud);
  if (uart_baud.mddr==0) err = R_SCI_UART_BaudCalculate(baudrate, false, err_rate, &uart_baud);
  err = R_SCI_UART_Open (&uart_ctrl, &uart_cfg);

Basically I expect bitrate_modulation to be true, so please don't think that setting it to false all the time will solve the problem.

@aentinger
Copy link
Contributor

Hi @mukyokyo ☕ 👋

Thank you for your investigation as well as sharing the results. Would you mind to prepare a PR for it?

@mukyokyo
Copy link
Author

mukyokyo commented Jul 5, 2023

The real purpose is different.
In the course of investigating the high number of dropouts of received data at high baudrate (over 200 kbps), I incidentally discovered that a baudrate mismatch was occurring.
When I am using my own send/receive routines with e2studio projects, I do not experience any overflow of received data even at several Mbps.
I felt that this fsp-dependent send/receive routine in the arduino library needs to be seriously modified.

In the meantime, I will leave the PR of this baudrate flaw to you.
I think the fsp team has been informed about the R_SCI_UART_BaudCalculate issue.

thanks

@facchinm
Copy link
Member

facchinm commented Jul 5, 2023

Hi @mukyokyo ,
I'd apply your patch anyway giving the proper attribution (so it would be great if you can prepare a PR), than we can tackle the overflow issues.
Please keep in mind that arduino APIs are supposed to be called by anyone in any context, so advanced but dangerous functionalities like DMA can't be used directly.

@mukyokyo
Copy link
Author

mukyokyo commented Jul 5, 2023

Please keep in mind that arduino APIs are supposed to be called by anyone in any context, so advanced but dangerous functionalities like DMA can't be used directly.

The uart HAL/Common Stacks was set in this library, but it assumes that DTC will be used (which doesn't seem to work in practice).
If you don't intend to use DMA, having this set up conflicts with your requirements.

The commonly used baudrate of 115.2 kbps or lower is still reasonably functional in its current state. Am I the only one who is not satisfied with this level of baudrate?

facchinm added a commit that referenced this issue Jul 7, 2023
Following the conversation with @mukyokyo on #25
Tested at 500000, 921600, 1M, 2M on UNO R4 WiFi
@mukyokyo
Copy link
Author

mukyokyo commented Jul 8, 2023

As for the missing received data, it is due to the interrupt priority, as I suspected.
I am not sure how IRQManager.h should be handled in Arduino, but changing UART_SCI_PRIORITY from 12 to 11 will reduce the overflow even at high baudrate(Limited to 1 Mbps or less.).

SCI generates an interrupt for every byte, so at high baudrate the load is very high.
If SCI-dependent applications require more performance in the future, the load will become higher and higher, and it may be difficult to cooperate with other applications.

I am not involved in the design of this library, but it is hard to come up with a good idea if the use of DMA is restricted.

@aentinger
Copy link
Contributor

Suggestions by @mukyokyo worked into 7b25c02 by @facchinm .

cristidragomir97 pushed a commit to cristidragomir97/ArduinoCore-renesas that referenced this issue May 20, 2024
USB: implement PluggableHID
Former-commit-id: 309e074
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

No branches or pull requests

3 participants