| Message ID | 20200708150934.13515-1-zajec5@gmail.com |
|---|---|
| State | Under Review |
| Delegated to: | Rafał Miłecki |
| Headers | show |
| Series | build: put DT "compatible" value as "board_name" in profiles.json | expand |
TL;DR: I think the issue is solved for devices using DT, the problem are the other targets with custom solutions. I think there is a policy for new DT devices to use the compatible string as profile. Multiple targets contain the following line in the target Makefile, which automatically adds the profile name as supported device: SUPPORTED_DEVICES := $(subst _,$(comma),$(1)) So ideally for all devices using DT, the profile and compatible string are the same except for '_' replaced by ','. For instance, the "Linksys WRT3200ACM" has the profile ID `linksys_wrt3200acm` and the automatically added compatible string `linksys,wrt3200acm`. So if that device wanted to search the `mvebu/cortexa9/profiles.json` for available sysupgrades, it takes the first entry from /proc/device-tree/compatible, replaces ',' with '_' find images in profiles.json['profiles']['linksys_wrt3200acm']['images']. For cases where DT compatible and OpenWrt profile ID/name where different either one was patched[0]. I think the question is therefore more on how to deal with devices that do not use DT? If we use a per device rootfs a line could be added to /etc/os-release containing something like OPENWRT_PROFILE="SpEcIaL-CaSEv100", which is then shown via `ubus call system borad`. Another hack could be to add it to /proc/cmdline? [0]: https://github.com/openwrt/openwrt/commit/df6f3090c48e3bafa0ace7450488b0a20a8074fb On 08.07.20 05:09, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@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@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 > > json_path.write_text(json.dumps(image_info, separators=(",", ":")))
> -----Original Message----- > From: Paul Spooren [mailto:mail@aparcar.org] > Sent: Mittwoch, 8. Juli 2020 21:34 > To: Rafał Miłecki <zajec5@gmail.com>; openwrt-devel@lists.openwrt.org > Cc: Rafał Miłecki <rafal@milecki.pl>; Adrian Schmutzler > <freifunk@adrianschmutzler.de>; Petr Štetiar <ynezz@true.cz>; Moritz > Warning <moritzwarning@web.de>; Daniel Golle <daniel@makrotopia.org> > Subject: Re: [PATCH] build: put DT "compatible" value as "board_name" in > profiles.json > > TL;DR: I think the issue is solved for devices using DT, the problem are the > other targets with custom solutions. > > I think there is a policy for new DT devices to use the compatible string as > profile. > > Multiple targets contain the following line in the target Makefile, which > automatically adds the profile name as supported device: > > SUPPORTED_DEVICES := $(subst _,$(comma),$(1)) > > So ideally for all devices using DT, the profile and compatible string are the > same except for '_' replaced by ','. > > For instance, the "Linksys WRT3200ACM" has the profile ID > `linksys_wrt3200acm` and the automatically added compatible string > `linksys,wrt3200acm`. So if that device wanted to search the > `mvebu/cortexa9/profiles.json` for available sysupgrades, it takes the first > entry from /proc/device-tree/compatible, replaces ',' with '_' > find images in profiles.json['profiles']['linksys_wrt3200acm']['images']. > > For cases where DT compatible and OpenWrt profile ID/name where > different either one was patched[0]. > > I think the question is therefore more on how to deal with devices that do > not use DT? If we use a per device rootfs a line could be added to /etc/os- > release containing something like OPENWRT_PROFILE="SpEcIaL-CaSEv100", > which is then shown via `ubus call system borad`. Well, one could just use something like this, which we had before the compatible was used: https://github.com/openwrt/openwrt/blob/openwrt-19.07/target/linux/ramips/base-files/lib/ramips.sh There, one could just use the "profile names" instead, with "_" replaced by "," for the few non-DT targets. However, this assumes a 1-to-1 relation between boards and profiles, and I'm not sure whether that always exists. And somebody would have to create that by hand. But I somehow lost track what's the ultimate goal of the proposed changes here, so maybe I'm not going into the right direction with this. Best Adrian > > Another hack could be to add it to /proc/cmdline? > > [0]: > https://github.com/openwrt/openwrt/commit/df6f3090c48e3bafa0ace7450 > 488b0a20a8074fb > > On 08.07.20 05:09, Rafał Miłecki wrote: > > From: Rafał Miłecki <rafal@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@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 > > > > json_path.write_text(json.dumps(image_info, separators=(",", ":")))
On 08.07.2020 21:34, Paul Spooren wrote: > I think there is a policy for new DT devices to use the compatible string as profile. > > Multiple targets contain the following line in the target Makefile, which automatically adds the profile name as supported device: > > SUPPORTED_DEVICES := $(subst _,$(comma),$(1)) > > So ideally for all devices using DT, the profile and compatible string are the same except for '_' replaced by ','. > > For instance, the "Linksys WRT3200ACM" has the profile ID `linksys_wrt3200acm` and the automatically added compatible string `linksys,wrt3200acm`. So if that device wanted to search the `mvebu/cortexa9/profiles.json` for available sysupgrades, it takes the first entry from /proc/device-tree/compatible, replaces ',' with '_' find images in profiles.json['profiles']['linksys_wrt3200acm']['images']. > > For cases where DT compatible and OpenWrt profile ID/name where different either one was patched[0]. There are still few exceptions like: linksys-ea9500 vs. compatible = "linksys,panamera" luxul-xwr-3150 vs. compatible = "luxul,xwr-3150-v1" luxul-xbr-4500 vs. compatible = "luxul,xbr-4500-v1" This is a bit of problem because: 1. I can't change "panamera" to "ea9500" as I was explicitly asked to stick to "panamera" by Imre when upstreaming that DTS 2. I don't want to rename "ea9500" to "panamera" to avoid: a. Confusing users b. Having meaningless filename So this is mostly what my patch is about. Providing reliable matching method that doesn't depend on profiles naming schema. I think adding 20 new lines of code for that purpose is reasonable. > I think the question is therefore more on how to deal with devices that do not use DT? If we use a per device rootfs a line could be added to /etc/os-release containing something like OPENWRT_PROFILE="SpEcIaL-CaSEv100", which is then shown via `ubus call system borad`. > > Another hack could be to add it to /proc/cmdline? I'd prefer to avoid forcing per device rootfs unless it's really required. We already have $(board_name) that results in unique device identifier on all (most?) targets. Let's stick to that and add matching entry in image/Makefile. My suggestion it to take existing Makefile entry like: define Device/linksys-e900-v1 DEVICE_MODEL := E900 DEVICE_VARIANT := v1 $(Device/linksys) DEVICE_ID := E900 VERSION := 1.0.4 endef and simpliy add BOARD_NAME := Linksys E900 V1 to it (it matches cat /proc/cpuinfo | grep Machine).
On 08.07.2020 21:41, mail@adrianschmutzler.de wrote: >> -----Original Message----- >> From: Paul Spooren [mailto:mail@aparcar.org] >> Sent: Mittwoch, 8. Juli 2020 21:34 >> To: Rafał Miłecki <zajec5@gmail.com>; openwrt-devel@lists.openwrt.org >> Cc: Rafał Miłecki <rafal@milecki.pl>; Adrian Schmutzler >> <freifunk@adrianschmutzler.de>; Petr Štetiar <ynezz@true.cz>; Moritz >> Warning <moritzwarning@web.de>; Daniel Golle <daniel@makrotopia.org> >> Subject: Re: [PATCH] build: put DT "compatible" value as "board_name" in >> profiles.json >> >> I think the question is therefore more on how to deal with devices that do >> not use DT? If we use a per device rootfs a line could be added to /etc/os- >> release containing something like OPENWRT_PROFILE="SpEcIaL-CaSEv100", >> which is then shown via `ubus call system borad`. > > Well, one could just use something like this, which we had before the compatible was used: > > https://github.com/openwrt/openwrt/blob/openwrt-19.07/target/linux/ramips/base-files/lib/ramips.sh That's exactly what I was thinking about. That file does: echo "$name" > /tmp/sysinfo/board_name which means device-specific identifier will appear as $(board_name) > There, one could just use the "profile names" instead, with "_" replaced by "," for the few non-DT targets. > However, this assumes a 1-to-1 relation between boards and profiles, and I'm not sure whether that always exists. > And somebody would have to create that by hand. Right. That list should be easy to verify by looking at Makefile and platform.sh / ramips.sh. > But I somehow lost track what's the ultimate goal of the proposed changes here, so maybe I'm not going into the right direction with this. I described it shortly in commit message as: > The purpose of "board_name" in JSON is matchine OpenWrt running device > with JSON profile entry. More details: we are working on firmware updating apps that find device appropriate firmware automatically. See luci-app-attendedsysupgrade for example. We need a way for such apps to: 1. Get device identifier (like $(board_name)) 2. Easily find matching firmware in profiles.json using above identifier
On Wed, Jul 08, 2020 at 11:32:43PM +0200, Rafał Miłecki wrote: > On 08.07.2020 21:34, Paul Spooren wrote: > > I think there is a policy for new DT devices to use the compatible string as profile. > > > > Multiple targets contain the following line in the target Makefile, which automatically adds the profile name as supported device: > > > > SUPPORTED_DEVICES := $(subst _,$(comma),$(1)) > > > > So ideally for all devices using DT, the profile and compatible string are the same except for '_' replaced by ','. > > > > For instance, the "Linksys WRT3200ACM" has the profile ID `linksys_wrt3200acm` and the automatically added compatible string `linksys,wrt3200acm`. So if that device wanted to search the `mvebu/cortexa9/profiles.json` for available sysupgrades, it takes the first entry from /proc/device-tree/compatible, replaces ',' with '_' find images in profiles.json['profiles']['linksys_wrt3200acm']['images']. > > > > For cases where DT compatible and OpenWrt profile ID/name where different either one was patched[0]. > > There are still few exceptions like: > linksys-ea9500 vs. compatible = "linksys,panamera" > luxul-xwr-3150 vs. compatible = "luxul,xwr-3150-v1" > luxul-xbr-4500 vs. compatible = "luxul,xbr-4500-v1" > > This is a bit of problem because: > 1. I can't change "panamera" to "ea9500" as I was explicitly asked to > stick to "panamera" by Imre when upstreaming that DTS I agree we should respect the artistic preferences of contributors to some degree. However, in this case, is there any reason for that somehow secretive naming scheme beyond personal preference? Having a consistent and easy to understand codebase also weights a bit higher than that to me. Cheers Daniel
On 2020-07-09 10:49, Daniel Golle wrote: > On Wed, Jul 08, 2020 at 11:32:43PM +0200, Rafał Miłecki wrote: >> On 08.07.2020 21:34, Paul Spooren wrote: >> > I think there is a policy for new DT devices to use the compatible string as profile. >> > >> > Multiple targets contain the following line in the target Makefile, which automatically adds the profile name as supported device: >> > >> > SUPPORTED_DEVICES := $(subst _,$(comma),$(1)) >> > >> > So ideally for all devices using DT, the profile and compatible string are the same except for '_' replaced by ','. >> > >> > For instance, the "Linksys WRT3200ACM" has the profile ID `linksys_wrt3200acm` and the automatically added compatible string `linksys,wrt3200acm`. So if that device wanted to search the `mvebu/cortexa9/profiles.json` for available sysupgrades, it takes the first entry from /proc/device-tree/compatible, replaces ',' with '_' find images in profiles.json['profiles']['linksys_wrt3200acm']['images']. >> > >> > For cases where DT compatible and OpenWrt profile ID/name where different either one was patched[0]. >> >> There are still few exceptions like: >> linksys-ea9500 vs. compatible = "linksys,panamera" >> luxul-xwr-3150 vs. compatible = "luxul,xwr-3150-v1" >> luxul-xbr-4500 vs. compatible = "luxul,xbr-4500-v1" >> >> This is a bit of problem because: >> 1. I can't change "panamera" to "ea9500" as I was explicitly asked to >> stick to "panamera" by Imre when upstreaming that DTS > > I agree we should respect the artistic preferences of contributors to > some degree. > However, in this case, is there any reason for that somehow secretive > naming scheme beyond personal preference? > Having a consistent and easy to understand codebase also weights a bit > higher than that to me. For Linksys, as said, I was told by Imre to use that magic "paramera" string. The only explanation I got was that it was "discussed for other platforms". For reference, see: [PATCH] ARM: dts: BCM5301X: Add basic DT for Linksys EA9500 https://patchwork.kernel.org/patch/9588095/ For Luxul devices "-v1" suffix was initially used (during development) and later it was decided V2 won't ever appear. So filename got simplified.
> -----Original Message----- > From: Rafał Miłecki [mailto:rafal@milecki.pl] > Sent: Donnerstag, 9. Juli 2020 11:12 > To: Daniel Golle <daniel@makrotopia.org> > Cc: Rafał Miłecki <zajec5@gmail.com>; Paul Spooren <mail@aparcar.org>; > openwrt-devel@lists.openwrt.org; Adrian Schmutzler > <freifunk@adrianschmutzler.de>; Petr Štetiar <ynezz@true.cz>; Moritz > Warning <moritzwarning@web.de>; Imre Kaloz <kaloz@dune.hu> > Subject: Re: [PATCH] build: put DT "compatible" value as "board_name" in > profiles.json > > On 2020-07-09 10:49, Daniel Golle wrote: > > On Wed, Jul 08, 2020 at 11:32:43PM +0200, Rafał Miłecki wrote: > >> On 08.07.2020 21:34, Paul Spooren wrote: > >> > I think there is a policy for new DT devices to use the compatible string > as profile. > >> > > >> > Multiple targets contain the following line in the target Makefile, which > automatically adds the profile name as supported device: > >> > > >> > SUPPORTED_DEVICES := $(subst _,$(comma),$(1)) > >> > > >> > So ideally for all devices using DT, the profile and compatible string are > the same except for '_' replaced by ','. > >> > > >> > For instance, the "Linksys WRT3200ACM" has the profile ID > `linksys_wrt3200acm` and the automatically added compatible string > `linksys,wrt3200acm`. So if that device wanted to search the > `mvebu/cortexa9/profiles.json` for available sysupgrades, it takes the first > entry from /proc/device-tree/compatible, replaces ',' with '_' find images in > profiles.json['profiles']['linksys_wrt3200acm']['images']. > >> > > >> > For cases where DT compatible and OpenWrt profile ID/name where > different either one was patched[0]. > >> > >> There are still few exceptions like: > >> linksys-ea9500 vs. compatible = "linksys,panamera" > >> luxul-xwr-3150 vs. compatible = "luxul,xwr-3150-v1" > >> luxul-xbr-4500 vs. compatible = "luxul,xbr-4500-v1" Essentially, we should be able to exploit SUPPORTED_DEVICES for that. Even if there is no sysupgrade with metadata available on the target, the SUPPORTED_DEVICES could indicate which board names would be possible for upgrade. This would also enable the possibility of a 1:n relationship (instead of a 1:1), i.e. one profile might be used for multiple "board names" (e.g. if you look at x86). The problem will arise from the opposite situation: e.g. in ar71xx, the same board name might be used for many profiles. With a board-name-based approach, we will never be able to support these for auto-upgrade or similar. However, I think ar71xx has been the last target that does this to a big extent. There are rare cases where the same DTS is used for more than one profile, but I think we can just ignore those for the moment. However, if we exploit the board name for that purpose, we should be aware that we move its use away from the initial intention (as in ar71xx, where it really described a board, not a device). I'm fine with that, as that's what I'm trying to enforce lately anyway, but not everyone might be. The implementation of "proper" board names for the non-DT targets then is not much more than some boring work to do (though e.g. bcm47xx could actually benefit from it). Best Adrian > >> > >> This is a bit of problem because: > >> 1. I can't change "panamera" to "ea9500" as I was explicitly asked to > >> stick to "panamera" by Imre when upstreaming that DTS > > > > I agree we should respect the artistic preferences of contributors to > > some degree. > > However, in this case, is there any reason for that somehow secretive > > naming scheme beyond personal preference? > > Having a consistent and easy to understand codebase also weights a bit > > higher than that to me. > > For Linksys, as said, I was told by Imre to use that magic "paramera" > string. The only explanation I got was that it was "discussed for other > platforms". For reference, see: > > [PATCH] ARM: dts: BCM5301X: Add basic DT for Linksys EA9500 > https://patchwork.kernel.org/patch/9588095/ > > For Luxul devices "-v1" suffix was initially used (during development) and > later it was decided V2 won't ever appear. So filename got simplified.
> -----Original Message----- > From: Rafał Miłecki [mailto:zajec5@gmail.com] > Sent: Mittwoch, 8. Juli 2020 23:40 > To: mail@adrianschmutzler.de; 'Paul Spooren' <mail@aparcar.org>; openwrt- > devel@lists.openwrt.org > Cc: 'Rafał Miłecki' <rafal@milecki.pl>; 'Adrian Schmutzler' > <freifunk@adrianschmutzler.de>; 'Petr Štetiar' <ynezz@true.cz>; 'Moritz > Warning' <moritzwarning@web.de>; 'Daniel Golle' <daniel@makrotopia.org> > Subject: Re: [PATCH] build: put DT "compatible" value as "board_name" in > profiles.json > > On 08.07.2020 21:41, mail@adrianschmutzler.de wrote: > >> -----Original Message----- > >> From: Paul Spooren [mailto:mail@aparcar.org] > >> Sent: Mittwoch, 8. Juli 2020 21:34 > >> To: Rafał Miłecki <zajec5@gmail.com>; openwrt-devel@lists.openwrt.org > >> Cc: Rafał Miłecki <rafal@milecki.pl>; Adrian Schmutzler > >> <freifunk@adrianschmutzler.de>; Petr Štetiar <ynezz@true.cz>; Moritz > >> Warning <moritzwarning@web.de>; Daniel Golle > <daniel@makrotopia.org> > >> Subject: Re: [PATCH] build: put DT "compatible" value as "board_name" > >> in profiles.json > >> > >> I think the question is therefore more on how to deal with devices > >> that do not use DT? If we use a per device rootfs a line could be > >> added to /etc/os- release containing something like > >> OPENWRT_PROFILE="SpEcIaL-CaSEv100", > >> which is then shown via `ubus call system borad`. > > > > Well, one could just use something like this, which we had before the > compatible was used: > > > > https://github.com/openwrt/openwrt/blob/openwrt- > 19.07/target/linux/ram > > ips/base-files/lib/ramips.sh > > That's exactly what I was thinking about. That file does: > echo "$name" > /tmp/sysinfo/board_name > which means device-specific identifier will appear as $(board_name) > > > > There, one could just use the "profile names" instead, with "_" replaced by > "," for the few non-DT targets. > > However, this assumes a 1-to-1 relation between boards and profiles, and > I'm not sure whether that always exists. > > And somebody would have to create that by hand. > > Right. That list should be easy to verify by looking at Makefile and platform.sh > / ramips.sh. > > > > But I somehow lost track what's the ultimate goal of the proposed changes > here, so maybe I'm not going into the right direction with this. > > I described it shortly in commit message as: > > > The purpose of "board_name" in JSON is matchine OpenWrt running > device > with JSON profile entry. > > More details: we are working on firmware updating apps that find device > appropriate firmware automatically. See luci-app-attendedsysupgrade for > example. > > We need a way for such apps to: > 1. Get device identifier (like $(board_name)) 2. Easily find matching firmware > in profiles.json using above identifier I just had a look into bcm47xx. Is my observation correct that, at least for mips74k subtarget, there also are multiple profiles for the same board, e.g. define Device/netgear-wnr3500l-v2 DEVICE_MODEL := WNR3500L DEVICE_VARIANT := v2 DEVICE_PACKAGES := $(USB2_PACKAGES) $(Device/netgear) NETGEAR_BOARD_ID := U12H172T00_NETGEAR NETGEAR_REGION := 1 endef TARGET_DEVICES += netgear-wnr3500l-v2 define Device/netgear-wnr3500-v2 DEVICE_MODEL := WNR3500 DEVICE_VARIANT := v2 DEVICE_PACKAGES := kmod-b43 $(Device/netgear) NETGEAR_BOARD_ID := U12H127T00_NETGEAR NETGEAR_REGION := 2 endef TARGET_DEVICES += netgear-wnr3500-v2 Do these really only differ in packages and region, or is there a parameter to determine which is used from the running device? Best Adrian
> -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] > On Behalf Of Rafal Milecki > Sent: Mittwoch, 8. Juli 2020 17:10 > To: openwrt-devel@lists.openwrt.org > Cc: Rafał Miłecki <rafal@milecki.pl>; Adrian Schmutzler > <freifunk@adrianschmutzler.de>; Petr Štetiar <ynezz@true.cz>; Moritz > Warning <moritzwarning@web.de>; Paul Spooren <mail@aparcar.org> > Subject: [PATCH] build: put DT "compatible" value as "board_name" in > profiles.json > > From: Rafał Miłecki <rafal@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@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. 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). Best Adrian > > json_path.write_text(json.dumps(image_info, separators=(",", ":"))) > -- > 2.26.1 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On 10.07.20 04:23, mail@adrianschmutzler.de wrote: >> -----Original Message----- >> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] >> On Behalf Of Rafal Milecki >> Sent: Mittwoch, 8. Juli 2020 17:10 >> To: openwrt-devel@lists.openwrt.org >> Cc: Rafał Miłecki <rafal@milecki.pl>; Adrian Schmutzler >> <freifunk@adrianschmutzler.de>; Petr Štetiar <ynezz@true.cz>; Moritz >> Warning <moritzwarning@web.de>; Paul Spooren <mail@aparcar.org> >> Subject: [PATCH] build: put DT "compatible" value as "board_name" in >> profiles.json >> >> From: Rafał Miłecki <rafal@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@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
Hi, > -----Original Message----- > From: Paul Spooren [mailto:mail@aparcar.org] > Sent: Montag, 13. Juli 2020 11:46 > To: mail@adrianschmutzler.de; 'Rafał Miłecki' <zajec5@gmail.com>; > openwrt-devel@lists.openwrt.org > Cc: 'Rafał Miłecki' <rafal@milecki.pl>; 'Petr Štetiar' <ynezz@true.cz>; 'Moritz > Warning' <moritzwarning@web.de> > Subject: Re: [PATCH] build: put DT "compatible" value as "board_name" in > profiles.json > > > On 10.07.20 04:23, mail@adrianschmutzler.de wrote: > >> -----Original Message----- > >> From: openwrt-devel [mailto:openwrt-devel- > bounces@lists.openwrt.org] > >> On Behalf Of Rafal Milecki > >> Sent: Mittwoch, 8. Juli 2020 17:10 > >> To: openwrt-devel@lists.openwrt.org > >> Cc: Rafał Miłecki <rafal@milecki.pl>; Adrian Schmutzler > >> <freifunk@adrianschmutzler.de>; Petr Štetiar <ynezz@true.cz>; Moritz > >> Warning <moritzwarning@web.de>; Paul Spooren <mail@aparcar.org> > >> Subject: [PATCH] build: put DT "compatible" value as "board_name" in > >> profiles.json > >> > >> From: Rafał Miłecki <rafal@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@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 Just to state that explicitly: That's a lot of work, and not something you quickly put together in one hour. > > 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? Shouldn't be too many, and I think I updated most of the DT targets to have consistent compatible/board name already. But one would have to check each of them (=target) and compile a list. > > 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. It's a matter of taste after all. However, using code names means that _everybody_ needs to do research to find the proper device. In contrast, if we just "arbitrarily" decided for one of the model names, 50 % of the user could just take the image and only the other half would have to learn a new name for their device. So, I'm still inclined to using model names and then just decide for one of them if necessary (or even add ALTx_ stuff for make menuconfig). (And if we had a very unpleasant situation where names are really misleading, we could still build the device twice.) > > Renaming the current profile called `linksys_viper` to > `linksys_e4200v2_and_ea4500` to have "user friendly" image names doesn't No, please not. > 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 only used it once for my old Samsung Galaxy S3, and back then I found it under this name. Best Adrian > > 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.1751 > 6-1-mail@aparcar.org/ > > [2]: https://lists.infradead.org/pipermail/openwrt-adm/2020- > June/001589.html
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 json_path.write_text(json.dumps(image_info, separators=(",", ":")))