Closed jeanguyomarch closed 8 years ago
Hi,
Indeed, this requirement must be fixed, thanks for the PR.
The patch creates a regression in the test suite, the gen_emptystr test was expected to fail.
Looking at this particular test, I still think it should fail but adding a single empty string should pass.
Could you change the length test instead of removing it add a new test in the suite to check if adding a single empty string does not fail ?
Hi,
Thanks for pointing out the failing test suite.
I'm not sure to have fully understood the changes you proposed on the test suite.
To me, the gen_emptystr
test seems valid (it should not fail), and the only change to bring to it would be to remove it from the list of failing tests.
Best regards, Jean
The behaviour of empty string is unclear, looking at the code it will create a node with 4 0´s bytes and will be decoded from dtb as a zero word attribute. In the same way, having an empty string along an correct string won't allow it to be decoded as a string attribute but an array of words.
Could you check the behaviour of dtc (feeding such dts, and reconverting back the dtb to dts) and if it's aligned, I'll accept the PR and disable the gen_emptystr test.
Neil
The empty string seems to have a well-defined behavior to me. Even though a string is empty, it is still an atom, so:
having an empty string along an correct string won't allow it to be decoded as a string attribute but an array of words
seems normal to me. To illustrate, below is a dummy DTS:
/dts-v1/;
/ {
compatible = "imx-ckih1", "fixed-clock";
empty = "";
empty2 = "", "";
compatible2 = "imx-ckih1", "", "", "fixed-clock";
compatible3 = "imx-ckih1", "", "", "fixed-clock", "";
};
after compiling to DTB and from the DTB to another DTS:
dtc -I dts -O dtb -o a.dtb a.dts
dtc -I dtb -O dts -o b.dts a.dtb
the DTS output is:
/dts-v1/;
/ {
compatible = "imx-ckih1", "fixed-clock";
empty = [00];
empty2 = [00 00];
compatible2 = "imx-ckih1", "", "", "fixed-clock";
compatible3 = "imx-ckih1", "", "", "fixed-clock", "";
};
with [???]
being a byte string (section 6.1). When reading section 2.2.4, those byte strings seem to match a string and a stringlist well.
Best regards, Jean
Thanks for the test, Could you also print the pyfdt dts output of this dtb ? I'm unsure about the compatible2 & 3 behaviour, it may need a sync with dtc.
Neil
Hi,
thank you for merging.
After passing the same dummy dts (compiled into a dtb with dtc
) to samples/dtbtodts.py
, I get:
/dts-v1/;
// version: 17
// last_comp_version: 16
// boot_cpuid_phys: 0x0
/ {
compatible = "imx-ckih1", "fixed-clock";
empty = [00];
empty2 = [00 00];
compatible2 = <0x696d782d 0x636b6968 0x31000000 0x66697865 0x642d636c 0x6f636b00>;
compatible3 = [69 6d 78 2d 63 6b 69 68 31 00 00 00 66 69 78 65 64 2d 63 6c 6f 63 6b 00 00];
};
and after converting it back to dtb and dts:
/dts-v1/;
/ {
compatible = "imx-ckih1", "fixed-clock";
empty = [00];
empty2 = [00 00];
compatible2 = "imx-ckih1", "", "", "fixed-clock";
compatible3 = "imx-ckih1", "", "", "fixed-clock", "";
};
So this seems fine.
Best regards, Jean
Thanks for the result, I still need to fix the string detection to correctly output the strings.
Hi,
The Devicetree specification (release 0.1) states in section 2.2.4 that:
Empty strings are therefore valid. They are also used in some devicetrees of the Linux kernel and other projects that also use device trees. Also,
dtc
is fine with empty strings.Best regards, Jean