Closed gckzl closed 1 year ago
@widgetii, I've checked that the code compiles, but I didn't get to test it on a device yet (my camera is currently bricked, I'm working on reflashing it, but it's gonna take a while, as it involves soldering). So, if the patch looks good to you, I'd be grateful if you could see if it works on a real device. Otherwise, I'll be able to test it myself in mid-August.
The wrong YAML output turned out to be a bug in the cYAML implementation, and it fundamentally couldn't handle nested lists correctly. I ended up rewriting the whole thing. Please take another look.
Small layout difference.
Edit: Seems like it is correctly done on the PR as it follows the same syntax.
Original:
rom:
- type: nor
block: 64K
partitions:
[...]
sensors:
- vendor: GalaxyCore
model: GC4653
control:
bus: 0
type: i2c
addr: 0x52
PR:
rom:
- type: nor
block: 64K
partitions:
[...]
sensors:
- vendor: GalaxyCore
model: GC4653
control:
bus: 0
type: i2c
addr: 0x52
In this case, we either render the "rom" section as it was, or the "sensors" section. I find the "rom" style to make more sense, but the "sensors" style seems to be specified on the YAML 1.2.2 spec, so I implemented the change. I would gladly revert this last change, though, spec be damned.
Hang on, in the last commit I've introduced a bug when rendering top-level lists. I'll see how I can fix it, but really, we should stick to the style in YAML 1.2.1 and earlier, and just drop that commit.
@widgetii, @viktorxda, what do you think?
Personally, as long as the syntax is conclusive I see no problem with both versions, but that needs to be decided by the maintainers. I can primarily give feedback or test the recent changes.
Here is the output of the different versions:
I've also fixed the handling of top-level lists, but I must say, my favorite is the "PR YAML 1.2.1", so I would drop these last two commits (would probably keep the tests).
Thank you, looks very good, I'll test everything on my devices this weekend
Hi @widgetii, did you get a chance to play with this PR? Also, any preference among the options presented in https://github.com/OpenIPC/ipctool/pull/79#issuecomment-1640202203?
Not yet, may be this weekend. If colleagues can make tests on their real devices, let's merge it early
Here is a output test from a ingenic device:
One thing to note is the slight delay between the execution and the output, as previously mentioned. Maybe you could add a message that indicates that the process is collecting the information.
Maybe you could add a message that indicates that the process is collecting the information.
In this case it will be not valid YAML anymore
I see. Otherwise I didn't notice any problems, here is a test sample from another device:
Ok, let's merge then
@gckzl thank for your efforts
@widgetii, @viktorxda Thanks for your help.
While working on the YAML code, I noticed it didn't handle empty strings, empty objects and empty arrays correctly: in that case, it would output invalid YAML (this limitation was present also before my re-write). I'll send a separate PR for this, as I didn't want to keep expanding this PR.
@gckzl
This PR breaks output of https://github.com/OpenIPC/ipctool/blob/master/src/boards/xm.c#L47 Before:
chip:
name: "XM_XM25QH128C"
id: 0x204018
After:
chip: " name: \"XM_XM25QH128C\"\n id: 0x204018\n"
@dimerr, sorry I hadn't realized nor_chip
contained YAML instead of a simple string.
I've sent out PR #128 with a fix.
upload
command, to make it clear the data is leaving the local networkOne side-effect of this change is that STDOUT isn't flushed anymore before trying to detect the sensors: all output happens after all the data has been collected.
Another change is that the output skips the initial "---" marker, making it a "bare document" in YAML terms. Since we're only producing a single document, this shouldn't make any difference.