sys/shell: fix missing generic lora settings in ifconfig #19466
Conversation
aabadie
added
the
Type: bug
aabadie
added
the
CI: ready for build
aabadie
force-pushed
the
pr/sys/shell_lora_ifconfig_fix
branch
from
e4c4c04 to
380b06f
Compare
aabadie
requested review from
leandrolanzieri,
jia200x,
MrKevinWeiss,
smlng,
MichelRottleuthner and
fjmolinas
as code owners
github-actions
bot
added
Area: drivers
|
I added a few more commits to that PR because I noticed other missing things:
I tested this PR on one of the nucleo-wl55jc board provided on the IoT-LAB Grenoble site and the initial problem is also fixed.
|
aabadie
force-pushed
the
pr/sys/shell_lora_ifconfig_fix
branch
from
380b06f to
f50be77
Compare
|
bors merge |
|
Build succeeded: |
|
This is backporting material, isn't it? |
|
Thanks for reviewing and acking!
I'm not sure, as this is quite a minor bug. |
|
I think we should introduce a |
and |
|
There are no lora phy specific commands. Everything is done via |
Actually
But isn't it the whole point of |
Not in the case of a lora driver because it provides other radio options, e.g BW/SF/CR. This PR only changes the LoRa settings that are displayed/can be changed using ifconfig. It should have no impact with gnrc lorawan specific behavior. The BW/SF/CR options are provided in the netdev adaptation layer at driver level (e.g phy).
I haven't tried to set/get GNRC lorawan options using ifconfig but I tried to keep the behavior when the |
This is what I mean: See that there's no On master: With this PR, it always brings the shell commands for LoRa.
It is not broken in the sense that it doesn't work, but it is not possible to disable the LoRa shell commands anymore when using LoRa. |
|
A fix could be as simple as renaming the old |
That would not solve anything and introduce confusion: the options that are added when We are also not talking about "lora shell commands" but rather ifconfig shell command options to configure a lora radio.
ok I think I get the point here. so maybe adding an intermediate module for "gnrc_netif_raw" and "lora" would make sense. But I don't understand why |
|
Please don't get me wrong, I'm not arguing for I'm just saying that before it was indeed possible to compile I don't have a strong opinion about naming, but IMO there should be at least one pseudomodule for turning on the "LoRa cmds of GNRC Netif" (which is why I proposed Regarding the pseudomodules, you would probably use the first one for the "raw" interface and the second for GNRC LoRaWAN, as you probably don't wanna change the PHY settings when the MAC is in control of the transceiver. You could though, if you pull in both modules. If we want to make it more generic, we could consider dropping the "gnrc" prefix from such pseudomodules, as we should aim to use stack independent components anyway (e.g netif/sock). |
Ok, I think I get the point. So by default, if a lora driver is used along with the shell_commands_default module, the LoRa options will be compiled in, otherwise, a "lora_shell_cmd" module must be used to pull them explicitly. |
Contribution description
This PR is fixing the missing bandwitdh, SF and CR LoRa parameter in the ifconfig command. In the
gnrc_netif.c, they are only available when usinggnrc_lorawanwhile they are more generally specific to LoRa (and there's a module for that).So this PR basically just fixes the module used to enable them.
Testing procedure
Run the
examples/defaulton a LoRa board, for example b-l072z-lrwan1 (there are a few available on iot-lab).On master, the BW, SF and CR are missing:
With this PR:
Issues/PRs references
I'm too lazy to track which PR introduced this regression (because it was working before)I found it: #18649