openwrt-openwisp-monitoring icon indicating copy to clipboard operation
openwrt-openwisp-monitoring copied to clipboard

[change] ModemManager device option needs to update to ctl_device

Open ghost opened this issue 4 years ago • 8 comments

Hey,

Just letting you know, we are changing this from device to ctl_device upstream in the OpenWrt ModemManager protocol handler because of compatibility issues with DSA. I will link the PR here when it is in.

https://github.com/openwisp/openwrt-openwisp-monitoring/blob/261239e7f6b0ff86876aebcb11292722e59c0c78/openwrt-openwisp-monitoring/files/lib/openwisp_monitoring/interfaces.lua#L21

ghost avatar Sep 19 '21 23:09 ghost

How's this look? It looks like the uci_cursor.get returns nil if that entry doesn't exist, right?

        local compat_device = uci_cursor.get('network', interface['interface'], 'device')
        local device = uci_cursor.get('network', interface['interface'], 'ctl_device')
        local modem = (compat_device or device)

        if (compat_device and device) then
            modem = device
        end

        local info = {}

        local general = io.popen('mmcli --output-json -m '..modem):read("*a")

mips171 avatar Sep 29 '21 01:09 mips171

@nickberry17 yes, it should, I haven't tested the following modified version yet but if it works is a bit shorter but still readable:

local device = uci_cursor.get('network', interface['interface'], 'ctl_device') or \
               uci_cursor.get('network', interface['interface'], 'device')
local info = {}
local general = io.popen('mmcli --output-json -m '..device):read("*a")

The second lineand the or won't be executed in the future and can be removed at some point in the future.

nemesifier avatar Sep 29 '21 01:09 nemesifier

Is it an inclusive or? Do we need to check if both are set, so we don't get device set to /sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1/sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1 In a sysupgrade scenario where settings are kept, both device and ctl_device could be set.

mips171 avatar Sep 29 '21 01:09 mips171

Is it an inclusive or? Do we need to check if both are set, so we don't get device set to /sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1/sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1 In a sysupgrade scenario where settings are kept, both device and ctl_device could be set.

The one with precedence should come first, the or is executed only if the first value is nil.

nemesifier avatar Sep 29 '21 01:09 nemesifier

@nickberry17 yes, it should, I haven't tested the following modified version yet but if it works is a bit shorter but still readable:

local device = uci_cursor.get('network', interface['interface'], 'ctl_device') or \
               uci_cursor.get('network', interface['interface'], 'device')
local info = {}
local general = io.popen('mmcli --output-json -m '..device):read("*a")

The second lineand the or won't be executed in the future and can be removed at some point in the future.

Didn't like the \

uci: Entry not found
lua: /usr/sbin/netjson-monitoring:326: unexpected symbol near '\'

Putting them on the same line:

uci: Entry not found
lua: /usr/sbin/netjson-monitoring:330: attempt to concatenate global 'modem' (a nil value)
stack traceback:
	/usr/sbin/netjson-monitoring:330: in function 'specialized_info'
	/usr/sbin/netjson-monitoring:392: in function 'get_interface_info'
	/usr/sbin/netjson-monitoring:578: in main chunk
	[C]: ?

The longhand way works though

mips171 avatar Sep 29 '21 01:09 mips171

Putting them on the same line:

uci: Entry not found
lua: /usr/sbin/netjson-monitoring:330: attempt to concatenate global 'modem' (a nil value)
stack traceback:
	/usr/sbin/netjson-monitoring:330: in function 'specialized_info'
	/usr/sbin/netjson-monitoring:392: in function 'get_interface_info'
	/usr/sbin/netjson-monitoring:578: in main chunk
	[C]: ?

What's the modem here? I think it is renamed to device in other prior instances expect one giving error

devkapilbansal avatar May 26 '22 21:05 devkapilbansal