[MP1] Add RPMsg virtual serial protocol support (VirtIOSerial)

[kbumsik]

Depends:

• [New variant] STM32MP157_DK  #717
• [MP1] Add upload tools for MP1: run_arduino_gen.sh and busybox-w32 Arduino_Tools#47

This PR currently includes commits from #717. Please look at commits with [VirtIO] title.

As #717 is almost being merged I propose VirtIOSerial as promised.
VirtIOSerial.cpp is a virtual serial based on RPMsg messaging protocol, which is built on top of VirtIO.
To clarify the terminologies the OpenAMP project standardizes RPMsg, VirtIO, and etc.

I named it VirtIOSerial because this is the most intuitive name for the most users. The code is based on OpenAMP_TTY_echo example from ST but it is heavily modified for Arduino Stream/Serial implementation.

The work is almost done and VirtIOSerial currently works well, but they are some minor issues needed to be addressed:

• OpenAMP list.h name may collision with freertos list.h OpenAMP list.h name collision with freertos list.h #760
  • I don't know how to check this properly yet.
• Arduino cannot send messages until Linux sends any data first. Can't RPMsg remote send messages until the master send the first message? OpenAMP/open-amp#182
• Clean some debugging messages (OPENAMP_log_dbg(), log_dbg()....)
• More documentations
• Maybe cleaning IAR/Keil specific directives (__ICCARM__, __CC_ARM) to reduce code noise?

This is the updated version of README.md

---

I'm currently testing with the simple example code below:

// the setup function runs once when you press reset or power the board
void setup() {
  Serial.begin(115200);
  // Serial7 is the standard Arduino serial pin (D0 and D1)
  Serial7.begin(115200);
  pinMode(LED_BUILTIN, OUTPUT);
}

// the loop function runs over and over again forever
void loop() {
  if(Serial.available()){
      while(Serial.available()){
        Serial.print(Serial.read());
      }
      Serial.println("");
//      Serial7.println("Gotcha");
  } else {
      Serial.println("No data");
//      Serial7.println("no data");
  }
  // Just blink
  digitalWrite(LED_BUILTIN, HIGH);
  delay(1000);
  digitalWrite(LED_BUILTIN, LOW);
  delay(1000);
}

You can compile & upload the script, and run sh run_arduino.sh start. After that you can test it by running minicom: $ TERM=xterm minicom -D /dev/ttyRPMSG0

Edit: there is a better Arduino Sketch to test:

int available;
char buffer[1024];

unsigned long time = 0;

void setup() {
  Serial.begin(115200);
  pinMode(LED_BUILTIN, OUTPUT);
}

void loop() {
  available = Serial.available();
  while (available > 0) {
    int size = min(available, Serial.availableForWrite());
    Serial.readBytes(buffer, size);
    Serial.write(buffer, size);
    available -= size;
  }

  // Heartbeat. If Arduino stops the LED won't flash anymore.
  if ((millis() - time) > 1000) {
    time = millis();
    digitalWrite(LED_BUILTIN, !digitalRead(LED_BUILTIN));
  }
}

---

This change is 

[fpistm]

Great!
Thanks @kbumsik !

[kbumsik]

A workaround for OpenAMP/open-amp#182 has been added.

Now run_arduino.sh has virtual serial related commands: stm32duino/Arduino_Tools@3f0c9a1

sh run_arduino.sh monitor
    Monitor data received from the coprocessor via the virtual serial.

sh run_arduino.sh send-msg <message...>
    Send a message to the coprocessor via the virtual serial.

sh run_arduino.sh send-file <filename>
    Send a file content to the coprocessor via the virtual serial.

sh run_arduino.sh minicom
    Launch minicom interactive serial communication program.

[fpistm]

• Maybe cleaning IAR/Keil specific directives (__ICCARM__, __CC_ARM) to reduce code noise?

I think it should be kept to avoid any issue with OpenAMP update.

If you talk about files copied in the core then yes they could be removed else ones in OpenAmp no to avoid any issue with OpenAMP update

[fpistm]

After a first quick review, here's my first feedback:

• I wonder if this should not be put as a builtin library?
• The VirtIO should be disabled by default
• I see no issue with list.h as OpenAmp requires to include it thanks: #include <metal/list.h>

[kbumsik]

@fpistm

• I wonder if this should not be put as a builtin library?

Do you mean splitting like other libraries such as STM32duinoBLE?

I know this library is very specific to the STM32MP1 series, but one of the main idea of the PR is to take over Serial. If a user wants to use Arduino for STM32MP1, they will likely expect that they can remove UART/USB for serial communications between the host (it was PC or Raspberry Pi, now it is the A7 Linux) and Arduino, and that is the major benefit of STM32MP1. By taking over Serial you can achieve this goal without any source code modification of existing Arduino sketches. Therefore VirtIO deserves to be pretty much the default Serial communication for STM32MP1.

I hope that VirtIO would be considered as the same level as USB CDC.

• The VirtIO should be disabled by default

Do you mean the default should be none like USB CDC does?

[kbumsik]

If you talk about files copied in the core then yes they could be removed else ones in OpenAmp no to avoid any issue with OpenAMP update

Yes I'm talking especially about rsc_table.c. They are just hard to read 😮

[fpistm]

@kbumsik

Do you mean splitting like other libraries such as STM32duinoBLE?

No, like a builtin library this is not the exactly the same.
I agree about Serial superseded but I think it should be possible. I will test and get back to you if possible.

Do you mean the default should be none like USB CDC does?

Yes

Yes I'm talking especially about rsc_table.c. They are just hard to read

So yes this could be cleaned

[fpistm]

@kbumsik
Just to keep you informed, I'm currently working to release the 1.8.0 and I will not have the time to well validate this PR. I will validate it for the next release.

[kbumsik]

@fpistm Please take your time, the holiday is coming :)

[ABOSTM]

@arnopo FYI

[ABOSTM]

Hi @kbumsik,
Thanks a lot for your PR, ... and your patience. I know it is quite a long time since you submitted this PR.
But I finally found time to make a deep review and to propose some update.
This is a huge work and it is well working. Thanks.

As I proposed some improvement, instead of making a review like usually, I propose a PR in your fork:
kbumsik#1
I kept your commits as is (except a rebase), and add new commits on top of yours. Of course, discussion is open about those proposed changes.

In addition, I have few questions/remarks:

1. In variants/STM32MP157_DK/README.md , you can remove limitation :
   * Currently there is no easy way for communication between the Linux host and Arduino coprocessor.
   And on the contrary add it as an implemented feature.

2. Question about RPMSG_BUFFER_SIZE:

#ifdef RPMSG_BUFFER_SIZE
#error "RPMSG_BUFFER_SIZE is already defined"
#else

Why raising an error? why not allowing user to redefine this value ? Is it linked to definition in linux driver ?
In this case, a comment would be welcome.

3. Is there any rational behind
   #define VIRTIO_BUFFER_SIZE (RPMSG_BUFFER_SIZE * 2) ?
   Is it made to deal with some kind of Half buffer management? link to below code:

if (prev_write_available < RPMSG_BUFFER_SIZE
     && virtio_buffer_write_available(&_VirtIOSerialObj.ring) >= RPMSG_BUFFER_SIZE) {
   MAILBOX_Notify_Rx_Buf_Free();
 }

If so, maybe you can add a comment before VIRTIO_BUFFER_SIZE definition and replace RPMSG_BUFFER_SIZE by VIRTIO_BUFFER_SIZE/2 in the checks:

if (prev_write_available < (VIRTIO_BUFFER_SIZE / 2)
      && virtio_buffer_write_available(&_VirtIOSerialObj.ring) >= (VIRTIO_BUFFER_SIZE / 2))

4. Add some header to new functions:
   Even if not always respected, we try to use a template from HAL, for example:

/**
  * @brief  Checks if there's an interrupt callback attached on Capture/Compare event
  * @param  channel: Arduino channel [1..4]
  * @retval returns true if a channel compare match interrupt has already been set
  */

[fpistm]

Hi @kbumsik,
I know this PR is old and probably you switch on other stuff.
Anyway, could you gave us a feedback or if you really could not give it, we probably do a clean PR with @ABOSTM update/review then close this one?
Let us know what you prefer.
Thanks in advance
BR

[kbumsik]

Hi @ABOSTM @fpistm,

I was just focused on other stuff until this week and now I can look into this PR again.

For 1 and 4, this PR currently needs a little bit of cleanup after receiving feedbacks of the initial design, so I will do them after the design is confirmed. Also note that this PR is a redesign of ST's OpenAMP_TTY example. So there are some uncleaned traces from the example code yet.

For 2,

Why raising an error? why not allowing user to redefine this value ? Is it linked to definition in linux driver ?

yes. The Linux kernel specified the buffer size as 512. You can see this here: https://elixir.bootlin.com/linux/v5.5.2/source/drivers/rpmsg/virtio_rpmsg_bus.c#L137

It was also a guard to make sure the definition is not overritten by rpmsg_virtio.h. I was originally tried to use rpmsg_virtio.h for RPMSG_BUFFER_SIZE but including this header causes problems that comes from the included headers of rpmsg_virtio.h. So I created

virtio_config.h and included a checker like this to make sure virtio_config.h is effective.

I think it is fine to remove this guard to let developers to adjust the size, as long as the dev don't forget to increase the buffer in the Linux kernel as well.

Maybe we can give #warn message instead of throwing an #error.

For 3,

Is it made to deal with some kind of Half buffer management?

No, also replacing it by VIRTIO_BUFFER_SIZE/2does not make sense. Whatever the size of VIRTIO_BUFFER_SIZE is it must call MAILBOX_Notify_Rx_Buf_Free() when the buffer has more than RPMSG_BUFFER_SIZE available.

The design of the ring buffer is influenced by cdc-queue.h. The ring buffer is introduced to make the original ST's example code mentioned above more general-purpose, and it increases responsiveness of the Linux's synchronous rpmsg channel but not Arduino.

virtio_rpmsg_bus.c                           virtqueue      vring   rpmsg_virtio.c
 ========   <-- new msg ---=============--------<------             ==========
||      || rvq (rx)    || IPCC CHANNEL 1 ||  svq (tx_vq) -> vring0 ||        || 
||  A7  ||  ------->-------=============--- buf free-->            ||   M4   ||
||      ||                                                         ||        ||
||master||  <-- buf free---=============--------<------            || slave  ||
||      || svq (tx)    || IPCC CHANNEL 2 ||  rvq (rx_vq) -> vring1 ||(remote)|| 
 ========   ------->-------=============----new msg -->             ==========

Let's look at the diagram here, the virtio / rpmsg / OpenAMP is implemented using IPCC interrupts and shared memory (MCU SRAM3 region).

Imagine A7 send a message to /dev/ttyRPMSG0. A7 sends a packet (max 512 bytes - 16 bytes header) to rvq virtqueue and trigger "new msg" interrupt. The message is stored in vring1 buffer which is associated with rvq. When copying vring1 to our Arduino virtio_buffer is done (thie is what rxCallback does), M4 need to trigger "buf free" interrupt back to A7 by calling MAILBOX_Notify_Rx_Buf_Free(). A7 sends another packet if the message is longer than 512 -16 bytes.

The important thing is that A7's sending the message to /dev/ttyRPMSG0 is fully synchronous operation. The A7 process is blocked until it receives "buf free" interrupt. If M4 don't trigger it in timely manner the A7 process will be blocked indefinitely until it gets kernel timeout error in the worst case. So my idea is to increase virtio_buffer to RPMSG_BUFFER_SIZE * 2 so that it can immediately send back "buf free" events until 2 full packet transmissions even if the Arduino application is not consuming the RX messages right now. We can adjust the size of the virtio_buffer but the code in question should be kept IMO.

I will look into your PR and will leave more feedbacks :)

[ABOSTM]

Hi @kbumsik
Thanks for your answer.
For 2, I am fine to keep this guard, just add a comment to explain it comes from linux setting.

[arnopo]

Minors comments then LGTM

[kbumsik]

Fixed 😛. Thanks for the review @arnopo 😉

[kbumsik]

@fpistm I made a mistake in the last commit "mbox_ipcc.c: rx_status_t -> mbox_status_t", which is fixed by the following fixup commit. However, the CI somehow passed the build of the failing commit. I guess the CI ignored board options. It should be fixed but I don't know how to locally test Github CI.

What is the best way to locally debug the new CI?

[fpistm]

I will check.

[fpistm]

I've found the issue. I've introduced a regression with #976 😖
Fixed in #982

[fpistm]

@kbumsik thanks again.
Just on last thing, please, could you do some cleanup in commits (squash,...)?
Thanks in advance.

[kbumsik]

@fpistm Thanks for approval. Sure, I kept them unclean to preserve the history but it's time to squash.
But, wow it's 58 commits. Allow me a day 😛

[fpistm]

@fpistm Thanks for approval. Sure, I kept them unclean to preserve the history but it's time to squash.
But, wow it's 58 commits. Allow me a day 😛

For sure 😉

[kbumsik]

/github/home/.arduino15/packages/STM32/hardware/stm32/1.8.0/CI/build/examples/BareMinimum/BareMinimum.ino:46:10: error: 'class VirtIOSerial' has no member named 'setRx'
   46 |   Serial.setRx(PIN_SERIAL_RX);
      |          ^~~~~

Well, it turns out to be it was a great idea to enable CI for VirtIOSerial.
So, should we just exclude VirtIO for that code block, like what USBD_USE_CDC did, or do we need to add mocks for setRx/setTx?

Edit: I excluded VirtIO from the failing lines in BareMinimum.ino. See a6dcefe. Tell me if you want to do a different approach.

[fpistm]

Well, it turns out to be it was a great idea to enable CI for VirtIOSerial.

Right, that's why I've updated also the CI sketch else this would not be raised.
That's fine for me. No reason to mock those methods.

Thanks for the update and the clean PR 👍 🥇

[ABOSTM]

LGTM. Great job. Thanks