Great!
Thanks@kbumsik !

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

Nowrun_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.

• 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

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 withlist.h as OpenAmp requires to include it thanks:#include <metal/list.h>

@fpistm

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

Do you mean splitting like other libraries such asSTM32duinoBLE?

I know this library is very specific to the STM32MP1 series, but one of the main idea of the PR is to take overSerial. 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 overSerial 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 benone like USB CDC does?

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 aboutrsc_table.c. They are just hard to read 😮

@kbumsik

Do you mean splitting like other libraries such asSTM32duinoBLE?

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 benone like USB CDC does?

Yes

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

So yes this could be cleaned

@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.

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

@arnopo FYI

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 aboutRPMSG_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 beforeVIRTIO_BUFFER_SIZEdefinition and replaceRPMSG_BUFFER_SIZEbyVIRTIO_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  */

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

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 ofST'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 byVIRTIO_BUFFER_SIZE/2does not make sense. Whatever the size ofVIRTIO_BUFFER_SIZE is it must callMAILBOX_Notify_Rx_Buf_Free() when the buffer has more thanRPMSG_BUFFER_SIZE available.

The design of the ring buffer is influenced bycdc-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 whatrxCallback does), M4 need to trigger "buf free" interrupt back to A7 by callingMAILBOX_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 toRPMSG_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 :)

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.

Minors comments then LGTM

Fixed 😛. Thanks for the review@arnopo 😉

@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?

I will check.

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

@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 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 😉

/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 whatUSBD_USE_CDC did, or do we need to add mocks forsetRx/setTx?

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

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 👍 🥇