sys/shell: adapt ifconfig to work with sx1272/76 devices by aabadie · Pull Request #7482 · RIOT-OS/RIOT

aabadie · 2017-08-17T15:38:11Z

As required in #7356, I adapted the ifconfig code to work with sx127x devices as well. Finally, not so difficult, but this code is becoming quite difficult to follow with all those #ifdefs.

Based on ~~#7356~~ ~~#8160~~ for the netif/auto_init adaption.

smlng · 2017-08-24T18:18:18Z

I see a problem with the proposed implementation here, at least the usage help text is either for LoRa with SX127x or for any other network device, also the command description for ifconfig is changed.

aabadie · 2017-08-24T19:14:09Z

I'm not sure to understand your comment.
The implementation of _usage is conditional (using macros) in regard to the type of device which is normal since LoRa devices (SX127x) are quite different from other devices, thus the usage message is different.
For the moment, only SX127x devices are supported for LoRa, this may change in the future if we provide support for other LoRa devices.

miri64 · 2017-08-24T19:16:56Z

What @smlng is saying is basically, what I was saying in #7356. It doesn't make sense, that LoRA gets a special command for interface configuration, since it is also just a network device / interface.

aabadie · 2017-08-24T19:42:13Z

Just pushed an update, let me know if it's better

smlng · 2017-08-24T20:11:57Z

since it is also just a network device / interface.

I'm not sure if it is that simple, but in general I agree.

However, judging from @aabadie s implementation it looks like that a LoRa interface has several additional attributes/parameters compared to ethernet or 15.4 interface while other are even missing or are not supported.

Maybe it does need an additional command to configure those special attributes, similar to iwpan on Linux for 15.4 or iwconfig for wifi a while back.

TL;DR this may need some thinking as the ifconfig command is getting quiet complex if it has to support diverse radios with loads of different and special parameters.

aabadie · 2017-09-04T11:22:14Z

Maybe it does need an additional command to configure those special attributes, similar to iwpan on Linux for 15.4 or iwconfig for wifi a while back.

What about a lora generic command ? It could have several subcommands:

lora config: list available lora interfaces, get/set parameters
lora send <if> <data>: send raw radio paquets
lora listen <if>: listen to raw radio incoming paquets
lora mac <join|tx|set|get>: configure the LoRamac layer (not available yet)

The get/set functions (and maybe others) could be shared between the actual ifconfig code and the lora code in a separate .c file.

miri64 · 2017-09-04T12:18:41Z

lora send : send raw radio paquet

Why is that one needed? What is different with lora, that txtsnd doesn't work?

lora listen : listen to raw radio incoming paquets

Same goes here.

aabadie · 2017-09-12T14:48:27Z

Why is that one needed? What is different with lora, that txtsnd doesn't work?

I didn't first of it initially and I just pushed an update with this change.

For the lora listen part, that's indeed true that it's not required since one can achieve the same result by putting the radio in RX state with ifconfig.

aabadie · 2017-09-19T16:53:59Z

@miri64, @smlng, do you other comments on this one ?

aabadie · 2017-12-07T13:09:43Z

@miri64, I would like to adapt this PR to #8159 when it's merged but before I want to be sure that it makes sense to modify the ifconfig/txtsnd commands to work with LoRa radio.
The ifconfig code also has changed after the netif remodeling so adapting it will probably end up in a lot of #ifdef.

miri64 · 2017-12-07T13:30:13Z

@miri64, I would like to adapt this PR to #8159 when it's merged but before I want to be sure that it makes sense to modify the ifconfig/txtsnd commands to work with LoRa radio.

When it comes to new netopts to be supported (IQ_INVERTED etc.) it makes sense. Yes!

The ifconfig code also has changed after the netif remodeling so adapting it will probably end up in a lot of #ifdef.

No, they don't require any #ifdefs or #ifndefs. If the option is not supported the network device returns -ENOTSUP so the command catches this. You don't see any #ifdefs for Ethernet or IEEE 802.15.4 in this here either, though they are very different protocols, right?

miri64 · 2017-12-07T13:30:46Z

Needs also adaptation for #7925.

miri64 · 2017-12-10T16:48:43Z

sys/include/net/netopt.h

    NETOPT_INTEGRITY_CHECK,

+    /**
+     * @brief   Get/Set the channel frequency as uint32_t.


Maybe add some detailed description (i.e. paragraph under this line), about how this differs from the NETOPT_CHANNEL option.

Ping @aabadie?

Not sure if better, but I added the detailed description.

Maybe not perfect, but sufficient for me.

miri64 · 2017-12-12T05:41:33Z

sys/shell/commands/sc_gnrc_netif.c

    if (res >= 0) {
        printf(" NID: 0x%" PRIx16, u16);
    }
+#ifdef MODULE_LORA


You removed the other #ifdefs. Why not this one (which except for size optimization is the least useful as it wouldn't show anything if compiled in but the options were not supported)?

miri64 · 2017-12-12T05:42:02Z

sys/shell/commands/sc_gnrc_netif.c

+    else if ((strcmp("frequency", key) == 0) || (strcmp("freq", key) == 0)) {
+        return _netif_set_u32(iface, NETOPT_CHANNEL_FREQUENCY, 0, value);
+    }
+#ifdef MODULE_LORA


Dito. Usage above isn't #ifdef'd

@miri64 I pushed 4ccfcfd that removes the ifdefs.

aabadie · 2017-12-12T16:49:34Z

@miri64 I guess we are good here, shall I squash ?

miri64

Minor complaint left

miri64 · 2017-12-13T09:41:31Z

sys/shell/commands/sc_gnrc_netif.c

+        hex = true;
+    }
+
+    if (res > 0xffffffff) {


UINT32_MAX? Also, I think it is safe to make this an assert for now. I don't know of any platform in our scope where sizeof(unsigned long) > sizeof(uint32_t).

aabadie · 2017-12-14T13:53:04Z

@miri64 shall I squash this one ?

miri64

Yes, please squash.

aabadie · 2017-12-14T14:02:29Z

squashed !

aabadie · 2017-12-14T15:17:32Z

@miri64, I have rom overflows issues with Murdock. Is it ok if I add some ifdefs to skip the LoRa specific functions ?

miri64 · 2017-12-14T15:24:29Z

No, just put the z1 into BOARD_INSUFFICIENT_MEMORY.

because of not enough memory

miri64 · 2017-12-14T15:29:49Z

But we definitely need a modular approach to ifconfig in the future. The current approach doesn't scale (as would #ifdef in such a case, see git log sys/transceiver/ to see the horrors that let us hate #ifdefs ;-)). Alternatively, extra commands to configure IEEE 802.15.4-, Ethernet-, LoRA-, ...-specific options separately as @jfischer-phytec-iot once proposed could help.

miri64 · 2017-12-14T15:32:57Z

(last proposal would be different than the original version of this PR would be that interface configuration from network layer view would still be done with ifconfig, regardless of the link-layer technology [compare iwconfig and ifconfig in Linux).

aabadie added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 17, 2017

aabadie added this to the Release 2017.10 milestone Aug 17, 2017

aabadie assigned miri64 Aug 17, 2017

aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 18, 2017

aabadie force-pushed the auto_init_sx127x_ifconfig branch from a26c953 to a923656 Compare August 19, 2017 20:08

aabadie force-pushed the auto_init_sx127x_ifconfig branch from a923656 to f44e78e Compare August 24, 2017 19:41

aabadie force-pushed the auto_init_sx127x_ifconfig branch from f44e78e to 017970f Compare August 28, 2017 12:19

aabadie force-pushed the auto_init_sx127x_ifconfig branch from 017970f to fd9b6eb Compare September 12, 2017 14:19

aabadie force-pushed the auto_init_sx127x_ifconfig branch from c9734ef to 98a2f91 Compare September 12, 2017 14:51

aabadie mentioned this pull request Sep 21, 2017

drivers/rn2xx3: basic initial implementation (without netdev) #7505

Merged

aabadie added the Area: LoRa Area: LoRa radio support label Oct 13, 2017

kYc0o mentioned this pull request Oct 29, 2017

Low-level Hardware Support #7888

Closed

23 tasks

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

aabadie mentioned this pull request Nov 28, 2017

sys/net/gnrc: provide lora adaption to netif #8158

Closed

aabadie force-pushed the auto_init_sx127x_ifconfig branch 2 times, most recently from 505aff6 to 4d7a4dd Compare December 9, 2017 18:37

aabadie force-pushed the auto_init_sx127x_ifconfig branch 2 times, most recently from a10df39 to 656e115 Compare December 10, 2017 16:26

miri64 requested changes Dec 10, 2017

View reviewed changes

aabadie force-pushed the auto_init_sx127x_ifconfig branch from 925e07a to a67a719 Compare December 11, 2017 07:59

aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Dec 11, 2017

aabadie force-pushed the auto_init_sx127x_ifconfig branch from a67a719 to 400ecd9 Compare December 11, 2017 18:17

miri64 requested changes Dec 12, 2017

View reviewed changes

aabadie force-pushed the auto_init_sx127x_ifconfig branch from 400ecd9 to 4ccfcfd Compare December 12, 2017 08:41

miri64 requested changes Dec 13, 2017

View reviewed changes

aabadie force-pushed the auto_init_sx127x_ifconfig branch from 4ccfcfd to 056aa7e Compare December 13, 2017 21:16

miri64 approved these changes Dec 14, 2017

View reviewed changes

aabadie added 5 commits December 14, 2017 15:01

makefiles: add lora pseudomodule

4c4e10f

drivers: load lora module with sx127x driver

4964150

sys/net/netopt: add NETOPT_CHANNEL_FREQUENCY option

5cf71e3

drivers/sx127x: use channel freq netopt

deb88bc

drivers/sx127x: make tx power types compliant with netopt

1461f31

aabadie force-pushed the auto_init_sx127x_ifconfig branch from 056aa7e to bfdd4c4 Compare December 14, 2017 14:01

sys/shell: adapt ifconfig to work with sx127x devices

d4d18d9

aabadie force-pushed the auto_init_sx127x_ifconfig branch from bfdd4c4 to d4d18d9 Compare December 14, 2017 15:13

tests/driver_enc28j60: exclude z1

5fc1324

because of not enough memory

miri64 merged commit d15c755 into RIOT-OS:master Dec 14, 2017

aabadie deleted the auto_init_sx127x_ifconfig branch February 26, 2018 10:32

Conversation

aabadie commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Aug 24, 2017

Uh oh!

aabadie commented Aug 24, 2017

Uh oh!

miri64 commented Aug 24, 2017

Uh oh!

aabadie commented Aug 24, 2017

Uh oh!

smlng commented Aug 24, 2017

Uh oh!

aabadie commented Sep 4, 2017

Uh oh!

miri64 commented Sep 4, 2017

Uh oh!

aabadie commented Sep 12, 2017

Uh oh!

aabadie commented Sep 19, 2017

Uh oh!

aabadie commented Dec 7, 2017

Uh oh!

miri64 commented Dec 7, 2017

Uh oh!

miri64 commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Dec 12, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Dec 14, 2017

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

aabadie commented Dec 14, 2017

Uh oh!

aabadie commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Dec 14, 2017

Uh oh!

miri64 commented Dec 14, 2017

Uh oh!

miri64 commented Dec 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aabadie commented Aug 17, 2017 •

edited

Loading

aabadie commented Dec 14, 2017 •

edited

Loading