Automatic On-Board GAP9 Deployment#178
Open
Victor-Jung wants to merge 12 commits intopulp-platform:develfrom
Open
Automatic On-Board GAP9 Deployment#178Victor-Jung wants to merge 12 commits intopulp-platform:develfrom
Victor-Jung wants to merge 12 commits intopulp-platform:develfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSelects GAP9 SDK config by SIMULATOR (board vs gvsoc), adds board deployment support and a gapy-based board runner, introduces optional GPIO power-measurement wiring, extends CLI/CMake to toggle power measurement and simulator, and switches test execution to streamed, ANSI-sanitized output with persistent logging. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DeeployTest/testUtils/testRunner.py (1)
293-304:⚠️ Potential issue | 🟡 MinorType hint and error message don't include 'board' simulator.
The
simulatorparameter'sLiteraltype hint (line 295) and the error message (line 303) don't include'board', but the runtime check (line 301) does. This inconsistency could confuse developers and cause type checker warnings.🔧 Proposed fix to sync type hint and error message
def __init__(self, platform: str, - simulator: Literal['gvsoc', 'banshee', 'qemu', 'vsim', 'vsim.gui', 'host', 'none'], + simulator: Literal['gvsoc', 'banshee', 'qemu', 'vsim', 'vsim.gui', 'host', 'board', 'none'], tiling: bool, argument_parser: TestRunnerArgumentParser, gen_args: str = "", cmake_args: str = ""): if simulator not in ['gvsoc', 'banshee', 'qemu', 'vsim', 'vsim.gui', 'host', 'board', 'none']: raise ValueError( - f"Invalid emulator {simulator} (valid options are 'gvsoc', 'banshee', 'qemu', 'vsim', 'vsim.gui', 'host', 'none')!" + f"Invalid emulator {simulator} (valid options are 'gvsoc', 'banshee', 'qemu', 'vsim', 'vsim.gui', 'host', 'board', 'none')!" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/testUtils/testRunner.py` around lines 293 - 304, In the TestRunner.__init__ function update the simulator type hint and error message to match the runtime check: add 'board' to the Literal type for the simulator parameter and include 'board' in the f-string listing valid options in the ValueError so the declared type, runtime validation, and error text are consistent (adjust the Literal in the __init__ signature and the string inside the ValueError message).
🧹 Nitpick comments (6)
DeeployTest/testUtils/core/execution.py (1)
224-224: Empty stderr passed toparse_test_outputmay cause missed error patterns.By merging stderr into stdout with
stderr=STDOUTand passing an empty string for stderr, any error patterns that only appear in stderr streams will be correctly captured inresult. However, theTestResultobject will have an emptystderrattribute, which may be misleading when accessed downstream (e.g., inpytestRunner.py).This is acceptable if stderr is intentionally merged, but consider documenting this behavior or setting
stderr=resultin the returnedTestResultif downstream consumers expect it populated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/testUtils/core/execution.py` at line 224, The call test_result = parse_test_output(result, "") leaves TestResult.stderr empty despite stderr being merged into stdout; update the code so parse_test_output receives the merged stderr value (e.g., pass result as the second argument) or, after parsing, set test_result.stderr = result so downstream consumers like pytestRunner.py see the combined stderr content; locate the call to parse_test_output in execution.py and change the argument or assign the stderr field on the resulting TestResult accordingly.DeeployTest/testUtils/deeployRunner.py (1)
410-411: Inconsistent-Dflag formatting.Line 411 uses
-D SIMULATOR=(with space) while line 408 uses-DPOWER_MEASUREMENT=ON(no space). While CMake accepts both forms, maintaining consistency improves readability.🧹 Proposed fix for consistent formatting
if platform == 'GAP9': - platform_specific_cmake_args.append("-D SIMULATOR=" + simulator) + platform_specific_cmake_args.append(f"-DSIMULATOR={simulator}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/testUtils/deeployRunner.py` around lines 410 - 411, The CMakelists flag formatting is inconsistent: change the string in the GAP9 branch so it uses the same no-space `-DNAME=VALUE` style as `-DPOWER_MEASUREMENT=ON`; update the conditional that appends to platform_specific_cmake_args (the `if platform == 'GAP9'` branch) to use "-DSIMULATOR=" + simulator instead of "-D SIMULATOR=" + simulator so the `platform_specific_cmake_args` entries are consistently formatted.DeeployTest/deeployRunner_tiled_gap9.py (1)
13-18: Consider extracting shared GAP9 argument setup.The
setup_parserfunction is duplicated verbatim betweendeeployRunner_gap9.pyanddeeployRunner_tiled_gap9.py. Consider extracting this to a shared module (e.g.,testUtils/platforms/gap9.py) to reduce duplication and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/deeployRunner_tiled_gap9.py` around lines 13 - 18, Duplicate setup_parser implementations in deeployRunner_gap9 and deeployRunner_tiled_gap9 should be extracted to a shared helper to avoid duplication; create a new module (e.g., testUtils.platforms.gap9) that exposes a function like setup_gap9_parser(parser) which adds the --cores and --powerMeasurement arguments, then replace the local setup_parser definitions in both deeployRunner_gap9.py and deeployRunner_tiled_gap9.py with an import and call to that shared setup_gap9_parser, ensuring the argument names and defaults (cores, powerMeasurement) remain identical.cmake/gap9/gap9_board.cmake (2)
59-75: Consider removing the no-opPOST_BUILDkeyword.
POST_BUILDis valid syntax foradd_custom_targetbut has no effect here—it's meaningful only foradd_custom_command. This could confuse readers into thinking there's post-build behavior.🔧 Suggested fix
add_custom_target(board_${name} DEPENDS ${name} WORKING_DIRECTORY ${BOARD_WORKDIR} COMMAND bash -c "${CMAKE_COMMAND} -E copy_if_different ${CMAKE_BINARY_DIR}/*.bin ${BOARD_WORKDIR}/ 2>/dev/null || true" COMMAND ${CMAKE_COMMAND} -E copy_if_different ${GAP9_SDK_HOME}/utils/efuse/GAP9/efuse_hyper_preload.data ${BOARD_WORKDIR}/chip.efuse_preload.data COMMAND ${CMAKE_COMMAND} -E echo "==========================================" COMMAND ${CMAKE_COMMAND} -E echo "[Deeploy GAP9] Executing gapy command to run on board:" COMMAND ${CMAKE_COMMAND} -E echo "${GAPY_CMD_STR}" COMMAND ${CMAKE_COMMAND} -E echo "==========================================" COMMAND ${GAPY_CMD} COMMENT "Running ${name} with gapy on GAP9 board" - POST_BUILD USES_TERMINAL VERBATIM )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/gap9/gap9_board.cmake` around lines 59 - 75, Remove the no-op POST_BUILD token from the add_custom_target invocation for board_${name}: edit the add_custom_target block that defines target board_${name} and delete the POST_BUILD argument (it’s meaningful only for add_custom_command), leaving the remaining COMMAND, COMMENT, USES_TERMINAL and VERBATIM settings unchanged so behavior is identical but the intent is clearer.
8-10: UnusedGVSOC_INSTALL_DIRcheck.This check validates
GVSOC_INSTALL_DIRbut this variable is never used within the macro. This appears to be leftover from copyinggap9_gvsoc.cmake. Consider removing it or replacing with a check that's actually needed for board deployment (e.g., verifying OpenOCD availability).🔧 Suggested fix - remove unused check
macro(add_board_deployment name target) - if(NOT DEFINED GVSOC_INSTALL_DIR) - message(FATAL_ERROR "Environment variable GVSOC_INSTALL_DIR not set") - endif() - message(STATUS "[Deeploy GAP9] Running on Board")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/gap9/gap9_board.cmake` around lines 8 - 10, The macro contains an unused check for GVSOC_INSTALL_DIR (if(NOT DEFINED GVSOC_INSTALL_DIR) ...), which is leftover and should be removed or replaced; update gap9_board.cmake by deleting the GVSOC_INSTALL_DIR existence check block or swap it for a relevant validation (for example, a check for OpenOCD availability like verifying OPENOCD_EXECUTABLE or running find_program) so the macro only tests variables actually used during board deployment.DeeployTest/Platforms/GAP9/src/deeploytest.c (1)
19-22: Consider documenting the GPIO pin selection.The hardcoded GPIO pin
89should be documented with a comment explaining why this specific pin is chosen (e.g., which physical pin this maps to on the GAP9 EVK, or reference to hardware documentation).📝 Suggested documentation
`#ifdef` POWER_MEASUREMENT +// GPIO 89 maps to GPIO_A89 on GAP9 EVK connector for external power measurement probe unsigned int GPIOs = 89; `#define` WRITE_GPIO(x) pi_gpio_pin_write(GPIOs, x) `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/Platforms/GAP9/src/deeploytest.c` around lines 19 - 22, The hardcoded GPIO pin value (unsigned int GPIOs = 89) used under POWER_MEASUREMENT and the WRITE_GPIO macro lack documentation; add a concise comment above the GPIOs/WRITE_GPIO definitions explaining why pin 89 was chosen (e.g., which GAP9 EVK physical header/pin it maps to, any power-measurement circuit it's tied to, and a reference to the hardware datasheet or board schematic), and keep references to the symbols GPIOs, WRITE_GPIO, POWER_MEASUREMENT, and the call to pi_gpio_pin_write in that comment so future readers can quickly locate and verify the mapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/gap9/gap9_board.cmake`:
- Line 6: The macro add_board_deployment currently accepts a parameter named
target but never uses it—replace the hardcoded "--target=gap9.evk" in the
commands inside the macro (references: macro add_board_deployment) with the
target parameter so the macro uses the provided target, or remove the unused
target parameter from the macro signature and update the call site in
DeeployTest/Platforms/GAP9/CMakeLists.txt accordingly; ensure any occurrence of
"--target=gap9.evk" inside the macro is replaced with the variable reference for
target if you choose Option 1.
- Around line 14-26: The code uses GAP9_SDK_HOME-derived variables (GAPY,
FLASH_LAYOUT, FSBL_BINARY, SSBL_BINARY) before verifying GAP9_SDK_HOME; move the
check so you validate that $ENV{GAP_SDK_HOME} is present (the variable set by
set(GAP9_SDK_HOME $ENV{GAP_SDK_HOME})) before constructing any paths, i.e.,
first set GAP9_SDK_HOME from the environment, immediately if(NOT DEFINED
GAP9_SDK_HOME) message(FATAL_ERROR ...), then afterwards set GAPY, FLASH_LAYOUT,
FSBL_BINARY, SSBL_BINARY and BOARD_WORKDIR/make_directory; update references to
the symbols GAP9_SDK_HOME, GAPY, FLASH_LAYOUT, FSBL_BINARY, and SSBL_BINARY
accordingly.
In `@DeeployTest/Platforms/GAP9/src/deeploytest.c`:
- Around line 86-92: The code performs two identical GPIO writes under
POWER_MEASUREMENT: first via pi_gpio_pin_write(GPIOs, 0) and then via
WRITE_GPIO(0); remove the redundant WRITE_GPIO(0) call so only
pi_gpio_pin_write(GPIOs, 0) remains (leave the surrounding POWER_MEASUREMENT
block, pi_pad_function_set(GPIOs, 1) and pi_gpio_pin_configure(GPIOs,
PI_GPIO_OUTPUT) intact) to avoid duplicate writes and keep a single
source-of-truth for GPIO operations.
In `@DeeployTest/testUtils/core/execution.py`:
- Around line 200-221: Replace the manual open/close and shell invocation: use a
with-context to open 'out.txt' (so fileHandle is always closed) and remove
shell=True by passing the existing cmd list directly to subprocess.Popen (not
cmd_str) when calling subprocess.Popen; keep reading from process.stdout as you
do, write escapeAnsi(output) into the file inside the with block, and delete the
stray fileHandle.write("") line; update references: replace uses of cmd_str and
fileHandle with cmd and the context-managed file handle, and keep process =
subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
encoding='utf-8') so subprocess.Popen, cmd, fileHandle, escapeAnsi,
config.test_dir and config.platform are updated accordingly.
In `@scripts/gap9-run.sh`:
- Around line 85-101: The resolver fallbacks are currently fatal under set -euo
pipefail because commands like getent hosts "$host", ip -4 addr show docker0,
and docker network inspect bridge can exit non‑zero; change the lookups to safe,
non‑fatal forms (use command substitution with conditional checks or append ||
true) so failures don't terminate the script, and ensure the getent path only
returns an IPv4 address by filtering the output with an IPv4 regex (e.g.,
awk/sed matching [0-9]+\.[0-9]+\.[0-9]+\.[0-9]+) before assigning to ip;
specifically update the getent hosts "$host" substitution, the ip -4 addr show
docker0 extraction, and the docker network inspect bridge extraction to be
tolerant of failure and to validate IPv4 prior to echo/return.
---
Outside diff comments:
In `@DeeployTest/testUtils/testRunner.py`:
- Around line 293-304: In the TestRunner.__init__ function update the simulator
type hint and error message to match the runtime check: add 'board' to the
Literal type for the simulator parameter and include 'board' in the f-string
listing valid options in the ValueError so the declared type, runtime
validation, and error text are consistent (adjust the Literal in the __init__
signature and the string inside the ValueError message).
---
Nitpick comments:
In `@cmake/gap9/gap9_board.cmake`:
- Around line 59-75: Remove the no-op POST_BUILD token from the
add_custom_target invocation for board_${name}: edit the add_custom_target block
that defines target board_${name} and delete the POST_BUILD argument (it’s
meaningful only for add_custom_command), leaving the remaining COMMAND, COMMENT,
USES_TERMINAL and VERBATIM settings unchanged so behavior is identical but the
intent is clearer.
- Around line 8-10: The macro contains an unused check for GVSOC_INSTALL_DIR
(if(NOT DEFINED GVSOC_INSTALL_DIR) ...), which is leftover and should be removed
or replaced; update gap9_board.cmake by deleting the GVSOC_INSTALL_DIR existence
check block or swap it for a relevant validation (for example, a check for
OpenOCD availability like verifying OPENOCD_EXECUTABLE or running find_program)
so the macro only tests variables actually used during board deployment.
In `@DeeployTest/deeployRunner_tiled_gap9.py`:
- Around line 13-18: Duplicate setup_parser implementations in
deeployRunner_gap9 and deeployRunner_tiled_gap9 should be extracted to a shared
helper to avoid duplication; create a new module (e.g.,
testUtils.platforms.gap9) that exposes a function like setup_gap9_parser(parser)
which adds the --cores and --powerMeasurement arguments, then replace the local
setup_parser definitions in both deeployRunner_gap9.py and
deeployRunner_tiled_gap9.py with an import and call to that shared
setup_gap9_parser, ensuring the argument names and defaults (cores,
powerMeasurement) remain identical.
In `@DeeployTest/Platforms/GAP9/src/deeploytest.c`:
- Around line 19-22: The hardcoded GPIO pin value (unsigned int GPIOs = 89) used
under POWER_MEASUREMENT and the WRITE_GPIO macro lack documentation; add a
concise comment above the GPIOs/WRITE_GPIO definitions explaining why pin 89 was
chosen (e.g., which GAP9 EVK physical header/pin it maps to, any
power-measurement circuit it's tied to, and a reference to the hardware
datasheet or board schematic), and keep references to the symbols GPIOs,
WRITE_GPIO, POWER_MEASUREMENT, and the call to pi_gpio_pin_write in that comment
so future readers can quickly locate and verify the mapping.
In `@DeeployTest/testUtils/core/execution.py`:
- Line 224: The call test_result = parse_test_output(result, "") leaves
TestResult.stderr empty despite stderr being merged into stdout; update the code
so parse_test_output receives the merged stderr value (e.g., pass result as the
second argument) or, after parsing, set test_result.stderr = result so
downstream consumers like pytestRunner.py see the combined stderr content;
locate the call to parse_test_output in execution.py and change the argument or
assign the stderr field on the resulting TestResult accordingly.
In `@DeeployTest/testUtils/deeployRunner.py`:
- Around line 410-411: The CMakelists flag formatting is inconsistent: change
the string in the GAP9 branch so it uses the same no-space `-DNAME=VALUE` style
as `-DPOWER_MEASUREMENT=ON`; update the conditional that appends to
platform_specific_cmake_args (the `if platform == 'GAP9'` branch) to use
"-DSIMULATOR=" + simulator instead of "-D SIMULATOR=" + simulator so the
`platform_specific_cmake_args` entries are consistently formatted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a36f195-ff1d-47e7-9951-64d7a090e367
📒 Files selected for processing (13)
CMakeLists.txtDeeployTest/Platforms/GAP9/CMakeLists.txtDeeployTest/Platforms/GAP9/sdk_board.configDeeployTest/Platforms/GAP9/sdk_gvsoc.configDeeployTest/Platforms/GAP9/src/deeploytest.cDeeployTest/deeployRunner_gap9.pyDeeployTest/deeployRunner_tiled_gap9.pyDeeployTest/testUtils/core/execution.pyDeeployTest/testUtils/deeployRunner.pyDeeployTest/testUtils/testRunner.pycmake/gap9/gap9_board.cmakecmake/gap9/gap9_gvsoc.cmakescripts/gap9-run.sh
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
DeeployTest/testUtils/core/execution.py (1)
200-205:⚠️ Potential issue | 🟠 MajorMissing
envparameter causes environment settings to be ignored.The
envdictionary is prepared on lines 186-196 withVERBOSEandBANSHEE_LOGsettings, but it's never passed tosubprocess.Popen(). This means verbosity and Banshee logging settings won't take effect during simulation.Additionally, the
shell=Truewith a joined command string is still present (previously flagged). Sincecmdis already a list, you can pass it directly toPopenwithout shell interpolation, which also avoids issues with paths containing spaces.,
🛠️ Proposed fix
- cmd_str = " ".join(cmd) - with subprocess.Popen(cmd_str, + with subprocess.Popen(cmd, stdout = subprocess.PIPE, stderr = subprocess.STDOUT, - shell = True, - encoding = 'utf-8') as process: + encoding = 'utf-8', + env = env) as process:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/testUtils/core/execution.py` around lines 200 - 205, The prepared env dictionary (built earlier) is never passed to subprocess.Popen and the code currently uses a joined cmd_str with shell=True; update the subprocess invocation in execution.py to call subprocess.Popen with the original cmd list (not cmd_str) so shell interpolation is not used, and pass env=env into Popen (keep encoding='utf-8' and stdout/stderr parameters). In short: replace usage of cmd_str and shell=True with passing cmd (the list) directly to subprocess.Popen and add the env=env argument to ensure VERBOSE/BANSHEE_LOG settings are applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@DeeployTest/testUtils/core/execution.py`:
- Around line 200-205: The prepared env dictionary (built earlier) is never
passed to subprocess.Popen and the code currently uses a joined cmd_str with
shell=True; update the subprocess invocation in execution.py to call
subprocess.Popen with the original cmd list (not cmd_str) so shell interpolation
is not used, and pass env=env into Popen (keep encoding='utf-8' and
stdout/stderr parameters). In short: replace usage of cmd_str and shell=True
with passing cmd (the list) directly to subprocess.Popen and add the env=env
argument to ensure VERBOSE/BANSHEE_LOG settings are applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02d7f3f4-972c-4654-b157-30d5cff14869
📒 Files selected for processing (1)
DeeployTest/testUtils/core/execution.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The goal of this PR is to facilitate on-board deployment on GAP9 from Deeploy. Note that there is no on-board CI yet.
Added
--powerMeasurementflag is present (using POWER_MEASUREMENT C macro).Changed
execution.pyto print the output of the simulation command in a streaming way.gap9-run.shto be agnostic of tmux pane naming (which depends on your own config).gap9-run.shfor the usbip bridge that works on any Linux/Macos/Wndows.PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.