tests: Add initial test case for Modbus Server Container #108

mgeeIOL · 2025-12-02T19:20:54Z

Description

Includes new test group for the modbus server, alongside the initial test case where the modbus server is coneceted to by a client on the github action's runner. Currently, the github actions runner connects to the modbus server to read and write to the LED and Button register(s). Will need to update test case as registers are implemented in the modbus server container.

This commit also includes a refactoring of parts of build.yml in the github actions to allow the actions runner to flash the board with the modbus server. Also changes the overall workflow slighly in order to make future container testing on the b_u585i_iot02a board easier to implement.

Type of change

Please delete options that are not relevant.

CI system update
Test Coverage update

How Has This Been Tested?

Github actions changes will be tested when the PR is opened, given that it is a change to how the GHA workflows are set up.

Modbus server tests have been ran manually over a serial connection to the board with the appropriate container flashed.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

mgeeIOL · 2025-12-02T19:22:31Z

Previous PR had some messy git history and unverified commit really early in the commit history. Recreated PR with relevant changes to clean up git history.

PatrickRobbIOL · 2025-12-02T22:07:36Z

.github/workflows/build.yml

 name: Build
+concurrency:
+  group: pr-workflows
+  cancel-in-progress: false


It's worth explaining this change in the commit message (we need to run workflows for different PRs in serial instead of synchronous since they are sharing a common resource).

PatrickRobbIOL · 2025-12-02T22:09:52Z

.github/workflows/build.yml

        sample:
-          - name: hello-world
-            expected: "powered by Ocre"
+          - name: generic-hello-world


I thought changing the name of the hello world artifact was out of scope but I guess it's okay given that we are extending usage of board specific container image (thus it makes sense to clarify what is board specific vs generic). So, I guess no changes needed.

PatrickRobbIOL · 2025-12-02T22:21:45Z

tests/groups/modbusServerValidation/modbus_server_validation_remote.py

+    conn.reset_input_buffer()
+    conn.send_break(duration=1)
+
+    # Wait for modbus server to be up and board to get networking


Maybe rephrase to "board to complete it's DHCP client process and initialize networking."

PatrickRobbIOL · 2025-12-02T22:23:26Z

tests/groups/modbusServerValidation/modbus_server_validation_remote.py

+    sys.exit(exitCode)
+
+"""
+This testcase is to be used following a modbus server being installed on a board.


"This testcase is to be run against a b_u585i_iot02a board which has its board specific modbus server container running on it."

PatrickRobbIOL · 2025-12-02T22:27:15Z

tests/groups/modbusServerValidation/modbus_server_validation_remote.py

+
+    print("----* Waiting for updates to Modbus server container before continuing *---")
+
+    # print("----* Test reading 32 bit floats *----") 


Remove commented out code. I realize you are just "waiting" for these aspects of the modbus server to get fixed, but let's not pushed commented out code. :)

PatrickRobbIOL

I left a few minor comments but it looks good. Approving this PR with the understanding that Matt will push an update per my comments. What do you think @kr-t ?

Includes new test group for the modbus server, alongside the initial test case where the modbus server is coneceted to by a client on the github action's runner. Currently, the github actions runner connects to the modbus server to read and write to the LED and Button register(s). Will need to update test case as registers are implemented in the modbus server container. This commit also includes a refactoring of parts of build.yml. Sets the concurrency stategy for running multiple PR workflows to be in serial rather than syncronous to prevent different runs affecting each other. Added the ability for the github actions runner to flash the b_u585i_iot02a board with the modbus server container. Also changes overall workflow slightly in order to make future container testing on the the b_u585i_iot02a board easier to implement. Signed-off-by: Matthew Gee <mgee@iol.unh.edu>

kr-t · 2025-12-05T08:31:09Z

Hi, can you merge the master/rebase so we can see that it works correctly with erase step?
That would also ensure that build merge errors are resolved.

…us-server-testing

Adds back a STM32_Programmer_CLI commands accidentally missed / deleted in previous merge. Signed-off-by: Matthew Gee <mgee@iol.unh.edu>

PatrickRobbIOL · 2025-12-08T21:43:56Z

@kr-t The actual content of this PR is fine, but do you need the commits re-organized in any way? When pulling from main, should we do a merge pull or rebase pull?

@mgeeIOL It looks to me like you did a merge pull instead of rebase pull (which might be okay, we just need to hear back from Krystian regarding what is preferred).

kr-t · 2025-12-09T12:46:55Z

Hi @PatrickRobbIOL , I prefer merge pull, since it keeps the history in tact and we'll be squashing it in the end anyway.
@SorinOlari helps me to review this, as the person implementing most CI and test related stuff in Ocre beside you guys.

SorinOlari · 2025-12-09T16:51:18Z

.github/workflows/build.yml

+            build-file: filesystem-full.wasm
            expected: "Directory listing for"
-            path: generic/filesystem-full
-          # Add here more samples


Can we add generic-blinky sample here?

SorinOlari · 2025-12-09T17:19:09Z

.github/workflows/build.yml

-  flash-zephyr-base-b_u585i_iot02a:
-    needs: build-zephyr-base
+  # Build and upload wasm files as artifacts
+  build-wasm-files:


I don't think it's necessary to do everything related to the application in this job. We can avoid these steps and theoretically save time. Will mark with '+' below places am I thinking about

SorinOlari · 2025-12-09T17:19:39Z

.github/workflows/build.yml


-      - name: Install build tools and WASI SDK
+      - name: Update Submodules
+        working-directory: application


SorinOlari · 2025-12-09T17:19:57Z

.github/workflows/build.yml

+      - name: Setup Zephyr project
+        uses: zephyrproject-rtos/action-zephyr-setup@v1
+        with:
+          app-path: application


SorinOlari · 2025-12-09T17:31:59Z

.github/workflows/build.yml

+            expected: "blink (count: 1, state: -)"
+          # Add here more samples
+    steps: 
+      - name: Install tools (xxd + WASI SDK)


This step is relevant to build-wasm-files, it should work without it

SorinOlari · 2025-12-09T17:34:41Z

.github/workflows/build.yml

+        with:
+          path: application
+
+      - name: Clone ocre-sdk


We are downloading .wasm file from artifacts, so ocre-sdk is unused here

SorinOlari · 2025-12-09T17:37:05Z

.github/workflows/build.yml

-          # Add here more samples
-    steps:
+    steps: 
+      - name: Install tools (xxd + WASI SDK)


the same thing as in build-and-run-linux-sample

SorinOlari · 2025-12-09T17:38:40Z

.github/workflows/build.yml

+        run: |
+          git submodule update --init --recursive
+
+      - name: Install tools (xxd + WASI SDK)


the same thing as above

SorinOlari · 2025-12-09T17:38:54Z

.github/workflows/build.yml


+
+  build-zephyr-modbus_server-b_u585i_iot02a:
+    needs: [build-wasm-files]


remove square brackets please

SorinOlari · 2025-12-09T17:46:50Z

I added some comments, I hope my theory is correct)
I think it would also be better not to save the .wasm files until the end, could you try deleting them after we used them?
Another option would be if we could save them all in a single artifact.
You could just try, until I think it's not a priority if that isn't part of the task @kr-t ?

PatrickRobbIOL · 2025-12-09T18:13:41Z

I added some comments, I hope my theory is correct) I think it would also be better not to save the .wasm files until the end, could you try deleting them after we used them? Another option would be if we could save them all in a single artifact. You could just try, until I think it's not a priority if that isn't part of the task @kr-t ?

All of your requested changes seem reasonable and simple. @mgeeIOL will work on it later today and push a new commit.

I just need a little clarification on the artifact point. Yes, I see there are about 10 artifacts on each GHA run now. Which files or dirs are the ones you actually care about preserving? I.e. if we build a WASM container image for blinky, is there value in us saving that as an artifact so that reviewers can download the blinky artifact, or is this just wasting space on the server maintaining this repo and its GHA runs?

Separately, @mgeeIOL please review and ensure that we are properly cleaning up on the Intel NUC i.e. are those tarballs we are wget'ing to meet dependencies getting cleared up when you run the generic cleanup workspace task?

SorinOlari · 2025-12-09T18:40:30Z

I just need a little clarification on the artifact point. Yes, I see there are about 10 artifacts on each GHA run now. Which files or dirs are the ones you actually care about preserving? I.e. if we build a WASM container image for blinky, is there value in us saving that as an artifact so that reviewers can download the blinky artifact, or is this just wasting space on the server maintaining this repo and its GHA runs?

I would go with the idea that it's a waste of space, I don't think the user needs .wasm files.
My initial idea was this:

build-wasm-files -> upload .wasm artifact -> build-and-run-platform-sample -> download .wasm artifact -> run it -> check expected log -> delete .wasm sample artifact

But now I realized that it is not really possible to delete the .wasm artifacts because we are using many matrixes, so we do not know when this artifact is ready to be deleted or not(and if it's possible in general).

kr-t · 2025-12-10T10:36:47Z

I think essentially, we shall think what the end-user would use.
This is the tipical current output vs this pr's:

The idea when we was thinking with @SorinOlari and @srberard about this was, that we shall keep the base minimum of useful applications, which are the firmware. It makes sense to just download built STM32U585 app (especially when we'll have ocre pull), as well as Linux's app. But frankly, nobody would ever download logs (you can check them in CI itself instead) or build .wasm apps, since they are mostly always customizable.

Therefore, at this point, I believe we shouldn't have anything in kept artifacts but the firmware images and app of the ocre-runtime.

Addresses comments brought up during review regarding unnecessary steps in the action workflow and minor syntax corrections. Still have to address the artifact storage method for .wasm files and their artifacts. (Squahsed with other commit to fix build errors arising from removing certain steps) Signed-off-by: Matthew Gee <mgee@iol.unh.edu>

Removed artifacting of log files and combined wasm artifact uploads into a singluar artifact. Currently the build wasm step saves all artifacts to a directory local on disk and they are artifacted in a seperate step. The directory used is cleaned at the end of the workflow to prevent runs from affecting each other. Signed-off-by: Matthew Gee <mgee@iol.unh.edu>

mgeeIOL · 2025-12-11T18:41:56Z

I have updated the PR to include the optimization changes commented on by @SorinOlari , let me know if I missed anything.

The logging artifacts are removed per the comment from @kr-t. As for removing the wasm artifacts I modified the workflow to have every wasm file be a part of the same artifact by saving them locally and uploading the directory as one artifact in a separate step.

This is the most practical option we have for handling the wasm artifacts as there is no official way to delete github artifacts and some jobs of the workflow require the wasm files and are not run on the zephyr-xlarge-runner so we cannot just store them locally.

To my knowledge the current artifacts are as close as we can get to the intention described earlier with the current workflow setup, if anything needs to change let me know.

SorinOlari · 2025-12-12T09:34:27Z

Looks good for me.
@kr-t

This reverts commit 533f2f5. This commit is kept in the legacy branch.

This reverts commit 533f2f5. This commit is kept in the legacy branch. Signed-off-by: Marco Casaroli <marco.casaroli@gmail.com>

mgeeIOL requested review from kr-t and srberard as code owners December 2, 2025 19:20

mgeeIOL mentioned this pull request Dec 2, 2025

[DRAFT] tests: Implement and add Modbus server testing #87

Closed

3 tasks

PatrickRobbIOL self-requested a review December 2, 2025 21:45

PatrickRobbIOL reviewed Dec 2, 2025

View reviewed changes

PatrickRobbIOL previously approved these changes Dec 2, 2025

View reviewed changes

mgeeIOL dismissed PatrickRobbIOL’s stale review via c4fb70f December 2, 2025 22:52

mgeeIOL force-pushed the test-modbus-server-testing branch from 06cf3b7 to c4fb70f Compare December 2, 2025 22:52

mgeeIOL added 2 commits December 5, 2025 13:57

Merge branch 'main' of github.com:mgeeIOL/ocre-runtime into test-modb…

bc2c86e

…us-server-testing

Fix board flash commands

5412336

Adds back a STM32_Programmer_CLI commands accidentally missed / deleted in previous merge. Signed-off-by: Matthew Gee <mgee@iol.unh.edu>

kr-t requested a review from SorinOlari December 9, 2025 12:47

SorinOlari requested changes Dec 9, 2025

View reviewed changes

mgeeIOL force-pushed the test-modbus-server-testing branch 3 times, most recently from 32e4f49 to 00a6eb6 Compare December 10, 2025 21:47

mgeeIOL force-pushed the test-modbus-server-testing branch from 00a6eb6 to cf6341a Compare December 10, 2025 21:57

kr-t approved these changes Dec 15, 2025

View reviewed changes

SorinOlari approved these changes Dec 15, 2025

View reviewed changes

kr-t merged commit 533f2f5 into project-ocre:main Dec 15, 2025
24 checks passed

casaroli added a commit that referenced this pull request Jan 7, 2026

Revert "tests: Add initial test case for Modbus Server Container (#108)"

2c95bc7

This reverts commit 533f2f5. This commit is kept in the legacy branch.

casaroli added a commit that referenced this pull request Jan 7, 2026

Revert "tests: Add initial test case for Modbus Server Container (#108)"

be71c57

This reverts commit 533f2f5. This commit is kept in the legacy branch. Signed-off-by: Marco Casaroli <marco.casaroli@gmail.com>

casaroli added a commit that referenced this pull request Jan 7, 2026

Revert "tests: Add initial test case for Modbus Server Container (#108)"

7e1b206

This reverts commit 533f2f5. This commit is kept in the legacy branch. Signed-off-by: Marco Casaroli <marco.casaroli@gmail.com>


		print("----* Waiting for updates to Modbus server container before continuing *---")

		# print("----* Test reading 32 bit floats *----")



		build-zephyr-modbus_server-b_u585i_iot02a:
		needs: [build-wasm-files]

tests: Add initial test case for Modbus Server Container #108

tests: Add initial test case for Modbus Server Container #108

Conversation

mgeeIOL commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

mgeeIOL commented Dec 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PatrickRobbIOL left a comment

Choose a reason for hiding this comment

Uh oh!

kr-t commented Dec 5, 2025

Uh oh!

PatrickRobbIOL commented Dec 8, 2025

Uh oh!

kr-t commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SorinOlari commented Dec 9, 2025

Uh oh!

PatrickRobbIOL commented Dec 9, 2025

Uh oh!

SorinOlari commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kr-t commented Dec 10, 2025

Uh oh!

mgeeIOL commented Dec 11, 2025

Uh oh!

SorinOlari commented Dec 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mgeeIOL commented Dec 2, 2025 •

edited

Loading

SorinOlari commented Dec 9, 2025 •

edited

Loading