[PATCH] build: put DT "compatible" value as "board_name" in profiles.json

[Paul Spooren]

On 10.07.20 04:23, mail at adrianschmutzler.de wrote:
>> -----Original Message-----
>> From: openwrt-devel [mailto:openwrt-devel-bounces at lists.openwrt.org]
>> On Behalf Of Rafal Milecki
>> Sent: Mittwoch, 8. Juli 2020 17:10
>> To: openwrt-devel at lists.openwrt.org
>> Cc: Rafał Miłecki <rafal at milecki.pl>; Adrian Schmutzler
>> <freifunk at adrianschmutzler.de>; Petr Štetiar <ynezz at true.cz>; Moritz
>> Warning <moritzwarning at web.de>; Paul Spooren <mail at aparcar.org>
>> Subject: [PATCH] build: put DT "compatible" value as "board_name" in
>> profiles.json
>>
>> From: Rafał Miłecki <rafal at milecki.pl>
>>
>> The purpose of "board_name" in JSON is matchine OpenWrt running device
>> with JSON profile entry. Right now it gets filled for devices using DT.
>> Other targets will require custom solutions or just speciyfing that value
>> manually.
>>
>> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
>> ---
>>   include/image.mk               |  3 +++
>>   scripts/json_add_image_info.py | 19 +++++++++++++++++++
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/include/image.mk b/include/image.mk index
>> 15f4fe9d3b..b33c1032f8 100644
>> --- a/include/image.mk
>> +++ b/include/image.mk
>> @@ -532,10 +532,13 @@ define Device/Build/image
>>   	@mkdir -p $$(shell dirname $$@)
>>   	DEVICE_ID="$(DEVICE_NAME)" \
>>   	BIN_DIR="$(BIN_DIR)" \
>> +	LINUX_DIR="$(LINUX_DIR)" \
>> +	KDIR="$(KDIR)" \
>>   	IMAGE_NAME="$(IMAGE_NAME)" \
>>   	IMAGE_TYPE=$(word 1,$(subst ., ,$(2))) \
>>   	IMAGE_PREFIX="$(IMAGE_PREFIX)" \
>>   	DEVICE_TITLE="$(DEVICE_TITLE)" \
>> +	DEVICE_DTS="$(DEVICE_DTS)" \
>>   	TARGET="$(BOARD)" \
>>   	SUBTARGET="$(if $(SUBTARGET),$(SUBTARGET),generic)" \
>>   	VERSION_NUMBER="$(VERSION_NUMBER)" \
>> diff --git a/scripts/json_add_image_info.py
>> b/scripts/json_add_image_info.py index b4d2dd8d71..5df4bf2a2a 100755
>> --- a/scripts/json_add_image_info.py
>> +++ b/scripts/json_add_image_info.py
>> @@ -5,6 +5,8 @@ from pathlib import Path  from sys import argv  import
>> hashlib  import json
>> +import re
>> +import subprocess
>>
>>   if len(argv) != 2:
>>       print("ERROR: JSON info script requires output arg") @@ -22,6 +24,20 @@
>> if not image_file.is_file():
>>   def get_titles():
>>       return [{"title": getenv("DEVICE_TITLE")}]
>>
>> +def get_board_name():
>> +    device_dts = getenv("DEVICE_DTS")
>> +    if device_dts is not None:
>> +        dtc = getenv("LINUX_DIR") + "/scripts/dtc/dtc"
>> +        dtb_file = getenv("KDIR") + "/image-" + device_dts + ".dtb"
>> +        dts = subprocess.run([dtc, "-q", "-I", "dtb", "-O", "dts", "-o", "-",
>> dtb_file], capture_output=True, text=True)
>> +        if dts.returncode != 0:
>> +            return None
>> +        match = re.search("compatible = \"([^\"]*)", dts.stdout)
>> +        if match is None:
>> +            return None
>> +        return match[1]
>> +    return None
>> +
>>
>>   device_id = getenv("DEVICE_ID")
>>   image_hash = hashlib.sha256(image_file.read_bytes()).hexdigest()
>> @@ -46,5 +62,8 @@ image_info = {
>>           }
>>       },
>>   }
>> +board_name = get_board_name()
>> +if board_name is not None:
>> +    image_info["profiles"][device_id]["board_name"] = board_name
> Hi,
>
> coming back to the initial subject of your patch:
>
> As stated earlier in the discussion (I think), we have "SUPPORTED_DEVICES" on many devices that will essentially give us the board name.
> So, one could say, the first entry of SUPPORTED_DEVICES just happens to be the board name (as that one is designed to match the current compatible ...).
>
> So, instead of establishing another mechanism to retrieve the board name in this patch (which might have several special cases etc.), I'd vote for just providing SUPPORTED_DEVICES for the remaining devices, even if it's not required for the upgrade mechanism itself.
I'm also in favor for using the first entry of SUPPORTED_DEVICES.
> This would allow us to benefit from what's set up already, and would allow to make target-specific solutions for the remaining cases (in some cases it might be just a one-liner in Device/Default).

I guess the only way which doesn't require per image root filesystems is 
to use a file like Adrian mentioned before[0] which ideally uses the 
very same schema as DT does (vendor_model).

[0]: 
https://github.com/openwrt/openwrt/blob/openwrt-19.07/target/linux/ramips/base-files/lib/ramips.sh

Adrian/Rafal do you have an overview which targets would be affected? 
Would it be okay to update them within the dev branch like previously 
done for some Linksys mvebu/cortexa9 devies?

Lastly I'd like to know your opinion on device codenames. As written 
some Linksys devices were renamed recently from codenames to model names 
(e.g. linksys,rango to linksys,wrt3200acm). This makes very much sense 
as long as only one device exists with a particular codename, however 
becomes messy if multiple models use the same board (e.g. boards with 
indoor or outdoor cases). Imre, presumably working together with Linksys 
and having some insight, responded with the following concern when I 
initially send the renaming patches to upstream:

> The Linksys Viper has been sold as E4200v2 and EA4500. The Linksys Focus as EA6100 and EA5800. The LeMans is the EA6300 and the EA6200. The Macan is both EA7500 and EA7400 - on the other hand, the EA7500v2 and the EA7400v2 are the Savannah.
So for e.g. the device Linksys E4200v2 / EA4500 (kirkwood) with codename 
Viper I see no sane way to reflect both models within the build system.

Renaming the current profile called `linksys_viper` to 
`linksys_e4200v2_and_ea4500` to have "user friendly" image names doesn't 
seem to cut it neither. I'm therefore wondering if we should stick in 
such cases to the codename and improve our documentation and user 
interfaces for such cases. Developers are capable to keep track of such 
codenames (yes, not super intuitive) and so are end users. It's maybe 
not a matter of less confusing filenames (else we would remove targets 
in filenames[1], right?) but of better documentation for the end user.

The very same works for the LineageOS project where downloaded firmware 
images always contains the codename only.

I might be entirely wrong here, some month ago I promoted the exact 
opposite approach by sending patches to the Kernel.

[1]: 
https://patchwork.ozlabs.org/project/openwrt/patch/20200614093330.17516-1-mail@aparcar.org/

[2]: https://lists.infradead.org/pipermail/openwrt-adm/2020-June/001589.html