backend: add nrfutil scraper backend - #64
Conversation
Implement NrfutilScraper (using nrfutil cli) and register nrfutil as an available runner in the CLI. Unfortunatelly nrfutil is a stateless tool so every memory read requires spawning a fresh process. Signed-off-by: Flavio Ceolin <flavio.ceolin@gmail.com>
wkhadgar
left a comment
wkhadgar
left a comment
There was a problem hiding this comment.
Thanks for making your first great contrib here! This looks really nice, I see this has no tests added for the readers, could you add tests like the other scrapers do? You can see the style in tests/test_msgq_walker.py and tests/test_cli.py
Really appreciate the time and effort put to help this project, thanks again
| if len(raw) < amount: | ||
| raw += b"\x00" * (amount - len(raw)) | ||
| return raw[:amount] |
There was a problem hiding this comment.
I worry that appending here with zeros might be a problem with the next steps since it would disguise a partial read for real data... For example if we are mid stack watermark reading and get the 0x00000000 instead of 0xAAAAAAAA assuming the stack was used, maybe the same happens for the pointer derefs that ZScraper do and so on. I would rather fail loudly here, and let the next layers grab the fail and handle it properly e.g.: the fatal error view.
There was a problem hiding this comment.
I copied this pattern from the gdb backend. To be honest I didn't make much sense to me it there as well, because you append "00" bytes but return only slice up to the amount read, just like here. But I am not super fluent and may had missed the reason and just followed the same thing. I am ok raising an error here if it read less data than requested.
There was a problem hiding this comment.
Actually a latent bug there too, if you want to open a follow up fix for it, i'd be glad to merge it, otherwise i can open it as soon as this merges
| detail = self._extract_error(proc.stdout) or proc.stderr.strip() or "no data returned" | ||
| raise ProbeReadError(f"nrfutil failed to read {amount}B at {hex(at)}: {detail}") | ||
|
|
||
| raw = bytes(values) |
There was a problem hiding this comment.
Since values comes from a process and not an API, I would make sure any fail here escalates as ProbeRead class, just for safety.
| zview -e build/zephyr/zephyr.elf -r jlink -t nRF5340_xxAA | ||
| ``` | ||
| **nrfutil (`-r nrfutil`)** | ||
| Pass the probe serial number. If `-t` is omitted, ZView selects devices with the `jlink` trait, which requires exactly one readable J-Link probe. |
There was a problem hiding this comment.
This only happens for the west zview invocation,-t is required for live/record on the standalone zview CLI and would not be in alignment with this statement, I suggest either make -t optional when -r nrfutil, or trim the doc and the fallback so we're not advertising something that can't run. Your call which way
2 participants
Implement NrfutilScraper (using nrfutil cli) and register nrfutil as an available runner in the CLI. Unfortunatelly nrfutil is a stateless tool so every memory read requires spawning a fresh process.