armv8m33: resolve problems with pyocd and hosts#446
Conversation
Summary of ChangesHello @maska989, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces necessary adjustments to support multicore debugging and improve reboot handling for ARMv8M33 targets, particularly when utilizing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly identifies the need for additional pyocd arguments for multicore support on the armv8m33 target and changes the serial port hint. The changes in flash_dut are a good step. However, to fully resolve the issues mentioned in the description, especially the reboot problem, the new multicore arguments (-Oconnect_mode=attach, -Oenable_multicore_debug=True) should also be applied to other PyocdProcess calls in the same file. Specifically, they seem to be missing in ARMv8M33Rebooter._reboot_by_debugger and MCXN947PloAppLoader.__call__. I've also added comments about using constants for hardcoded strings to improve maintainability.
mateusz-bloch
left a comment
mateusz-bloch
left a comment
There was a problem hiding this comment.
LGTM, only comments about cosmetics.
trunner/target/armv8m33.py
Outdated
| PyocdProcess( | ||
| target="mcxn947", extra_args=["--format=bin", "--erase=chip"], host_log=host_log, cwd=self.boot_dir() | ||
| target="mcxn947", | ||
| extra_args=["--format=bin", "--erase=chip", "-Oconnect_mode=attach", "-Oenable_multicore_debug=True"], |
There was a problem hiding this comment.
It may be worth adding a comment explaining why additional arguments are needed?
There was a problem hiding this comment.
I also think, it's not so obvious that we need "-Oconnect_mode=attach", "-Oenable_multicore_debug=True"
There was a problem hiding this comment.
# connect to a running target without halting cores.
It's not showing why it's needed, but what is happening. According to our golden rules, described on YT page: code shouldn't need comments regarding WHAT you're doing, only WHY You're doing something
# The primary effect is to modify the default software reset type
# for secondary cores to use VECTRESET, which will fall back to emulated
# reset for secondary cores that are not v7-M architecture
Too long explanation
I think that comment like # additional options needed to allow flashing without hard reboot and support cpu1 configuration would be completely fine - placed just above the extra_args line
There was a problem hiding this comment.
Longer explanation you should leave in the commit description
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
3255533 to
c539bbc
Compare
|
If I understand correctly - this is not a simple "Bug fix", You've changed the hardware configuration of the board (added extra usb-serial adapter)? This should be clearly stated in the code (that we're using external adapter because the MCU VCOM fails when blah blah blah). Not sure this is the correct approach - this change makes it harder to use the test runner outside of CI. I would expect the MCU link to be enough to collect the logs correctly, if there is a problem during reboot - probably the reboot procedure might be improved instead of adding additional HW. |
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
c539bbc to
d569c59
Compare
Unfortunately we have to choose:
Last time we had such problem, we chose together the option with external converter (imx6ull-evk target), but another opportunity has appeared - as discussed with @maska989 new options for pyocd made flashing without power off possible - that's why I would strongly recommend the approach with abandoning hard reboots on this target to unify the environment and make running on host pc possible without any changes. |
damianloew
left a comment
damianloew
left a comment
There was a problem hiding this comment.
Finally the change with find_port will be abandoned, but had you wanted to add both, you would have needed 2 separate commits - switch to external usb-uart converer and trunner: extend the pyocd options in the flashing command on mcx target. Of course in such changes commit description with longer WHY is strongly recommended - even required
EDIT: Finally you will have to add a second commit anyway, because dismissing hard reboot is also a separate change
Small comment about commit title: trunner: prefix we are always using when changing trunner code, armv8m33 - I'd suggest using the tatget name, so mcx in this case - in the future we could have more armv8m33 targets and commit history could be misleading then
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
d569c59 to
eb4970c
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
maska989
changed the title
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
2 times, most recently
from
1e52ff2 to
c55874b
Compare
Sadly, after further investigation there is still some kind of error and rebooting it with power off is the only possible way to make target runs consistent in time. Part of the problem discussed here: https://community.arm.com/support-forums/f/keil-forum/57462/issue-with-pyocd-flashing-with-cmsis-pack-nxp-mcxn947_dfp-25-09-00. Downgrading version is not easy process but still manageable, but still doesn't bring solution because other versions still have problems with consistent connections |
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
c55874b to
da676f1
Compare
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
da676f1 to
e7a3a2e
Compare
mateusz-bloch
left a comment
mateusz-bloch
left a comment
There was a problem hiding this comment.
LGTM, only correct commit message :P
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
e7a3a2e to
32f5b6b
Compare
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
32f5b6b to
38694f8
Compare
trunner/target/armv8m33.py
Outdated
| cwd=self.app_host_dir, | ||
| ).load(load_file=app.file, load_offset=self.ram_addr + offset) | ||
|
|
||
| # There is hardware difference between test unit and out of the box unit |
There was a problem hiding this comment.
What difference? The behavior should be the same both for rpi and host pc.
There was a problem hiding this comment.
It seems that on rpi, loading the test binary causes reboot and we have to enter plo once again, on host-pc not. I understand that hw difference is an external uart usb converter with a diode (?), but it shouldn't cause board reset.
There was a problem hiding this comment.
There is a difference between a device straight out of the box and one that has been modified for continuous and uninterrupted operation in a test environment.
The first modification, as you mentioned, is the addition of an external UART to prevent loss of logs and to maintain connectivity during hard reboots.
The second important modification that was implemented involves disabling the possibility of powering the FRDM board from the MCU-LINK communication port. This was achieved by removing the diode on the power supply line. However, this change may cause issues when connecting via pyOCD. Depending on the version, older releases did not include an authentication procedure, which meant that application loading did not trigger reboots.
| # For consistent testing without any error we're using full power down sequence | ||
| if self.host.has_gpio(): |
There was a problem hiding this comment.
If I understand correctly:
| # For consistent testing without any error we're using full power down sequence | |
| if self.host.has_gpio(): | |
| # Without a hard reboot before flashing, intermittent failures occurred after several consecutive test campaigns | |
| if self.host.has_gpio(): |
AFAIK it is not seen on host-pc, because usually we power up the board a moment before running sth on it.
Btw it looks like the issue - the board should work stable during many campaigns without a hard reboot, have you investigated this? Maybe sth is wrong in our system and that's the reason - issue should be raised in phoenix-rtos-project then.
There was a problem hiding this comment.
This is not strictly an issue with our environment, but rather with unpredictable interactions between pyOCD and the FT2232H located on the FRDM board, for two key reasons:
pyOCD does not natively provide a library for supporting the MCXN947; this support is developed by the community, and the implemented solutions are not always correct or fully reliable.
Modifications related to the power supply path between the FT2232H and the CPU also affect the behavior of pyOCD.
Failures occurred in this case scenario are connected with "cannot go to bootloader mode".
trunner/target/armv8m33.py
Outdated
| PyocdProcess(target="mcxn947", extra_args=["--format=bin", "--no-reset"], cwd=self.app_host_dir).load( | ||
| load_file=app.file, load_offset=self.ram_addr + offset | ||
| ) | ||
| # In this approach, pyocd remember last used configuration with connection after flashing sequence |
There was a problem hiding this comment.
configuration with connection after flashing sequence - what do you mean? It's not clear. Adding -Oenable_multicore_debug=True caused that?
remembers*
Please update the comment to be more descriptive (not longer)
There was a problem hiding this comment.
My apologies — this is a leftover from a previous code iteration, and the old comment remained. It has already been removed.
trunner/target/armv8m33.py
Outdated
| target="mcxn947", extra_args=["--format=bin", "--erase=chip"], host_log=host_log, cwd=self.boot_dir() | ||
| cwd=self.boot_dir(), | ||
| host_log=host_log, | ||
| # additional options needed to allow flashing without hard reboot and support cpu1 configuration |
There was a problem hiding this comment.
Without hard reboot? How?
There was a problem hiding this comment.
I explained it incorrectly — I meant tests performed without the test environment (standalone).
Comment removed.
Adding multicore setup and edit connection way with pyocd JIRA: CI-592
maska989
force-pushed
the
maska989/mcxn94x_tweaks
branch
from
38694f8 to
887e458
Compare
4 participants
Description
To fully support multicore connection, we need to use additional options with pyocd which allows to multicore debugging (The primary effect is to modify the default software reset type for secondary core to use VECTRESET, which will fall back to emulated reset for secondary cores that are not v7-M architecture) and granting access to reboot differently each core.
To make possible execution campaign without any hick ups and make it consistent, we need to perform hard reboot before flash step also to collect more logs we need to use external UART it gives us possibility to read logs between hard restarts and prevent from changing connected ACM like it have place with build in debug chip.
Additional pyocd options:
It is also good to mention to not use hard link to debug type (Cause troubles) via "-Oreset_type=" but in some scenarios can help with connections problems
Incorrect reboot and connect handling for core1:
Correct reboot and connect handling for core1:
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment