| Message ID | 20181219155616.9547-5-ben.whitten@lairdtech.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Get sx1301 to transmit lora packets | expand |
Hi Ben, Am 19.12.18 um 16:56 schrieb Ben Whitten: > Information such as spreading factor, coding rate and power are on a per > transmission basis so it makes sence to include this in a header much "sense" (even in British English! :)) > like CAN frames do. Any pointer for that? My understanding of CAN was that it actually uses the CAN or CAN FD frame for transmission on the wire. Whereas here you use it for metadata that does not go over the air. > > Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com> > --- > drivers/net/lora/dev.c | 2 - > drivers/net/lora/sx1301.c | 79 +++++++++++++++++++++++++++++++++------ > include/uapi/linux/lora.h | 46 +++++++++++++++++++++++ > 3 files changed, 114 insertions(+), 13 deletions(-) If we should seriously consider this new approach, please always cleanly split it off from sx1301 driver changes. The most important changes are at the very bottom here otherwise. Some of the enums may be useful either way. [snip] > diff --git a/include/uapi/linux/lora.h b/include/uapi/linux/lora.h > index 4ff00b9c3c20..d675025d2a6e 100644 > --- a/include/uapi/linux/lora.h > +++ b/include/uapi/linux/lora.h > @@ -21,4 +21,50 @@ struct sockaddr_lora { > } lora_addr; > }; > > +#define LORA_MAX_DLEN 256 Note: According to Helmut, "SX1276 can do MTU sizes up to 2048 bytes". But I have failed to find mention of LoRa-mode interrupts other than TxDone or any such how-to in the SX1276 manual or on Google; only the FSK/OOK modem has a FIFO-less Continuous mode documented. > + > +enum lora_bw { > + LORA_BW_125KHZ, > + LORA_BW_250KHZ, > + LORA_BW_500KHZ, Lacking lower bandwidths. SX127x can go as low as 7.8 kHz. Would it work to have it as an integer in Hz for flexibility? > +}; > + > +enum lora_cr { > + LORA_CR_4_5, > + LORA_CR_4_6, > + LORA_CR_4_7, > + LORA_CR_4_8, > +}; If we have this as ABI, we should probably go safe and initialize the first one to 0. > + > +enum lora_sf { > + LORA_SF_6, > + LORA_SF_7, > + LORA_SF_8, > + LORA_SF_9, > + LORA_SF_10, > + LORA_SF_11, > + LORA_SF_12, > +}; Wouldn't an integer be sufficient for the Spreading Factor? We'll need to safeguard code against unexpected values anyway. An added bonus for the enum/defines would be if we could have LORA_SF_6 = 64 up to LORA_SF_12 = 4096 (chips per symbol) > + > +/** > + * struct lora_frame - LoRa frame structure > + * @freq: Frequency of LoRa transmission in Hz > + * @power: Power of transmission in dBm > + * @bw: bandwidth, 125, 250 or 500 KHz > + * @cr: coding rate, 4/5 to 4/8 > + * @sf: spreading factor from SF6 to 12 > + * @data: LoRa frame payload (up to LORA_MAX_DLEN byte) > + */ > +struct lora_frame { > + __u32 freq; /* transmission frequency in Hz */ > + __u8 power; /* transmission power in dBm */ > + enum lora_bw bw; /* bandwidth */ > + enum lora_cr cr; /* coding rate */ > + enum lora_sf sf; /* spreading factor */ > + __u8 len; > + __u8 data[LORA_MAX_DLEN] __attribute__((aligned(8))); > +}; I'm not comfortable with making this arbitrary format a userspace ABI. For example, hardcoding the frequency as __u32 will limit us to 4 GHz, which is sufficient for 2.4 GHz, but Wifi is already at 5 and 60 GHz. Having the data at the end means we can't easily add header fields, such as Sync Word (that you're missing above) or a frequency extension word. Reuse with FSK/OOK or other LPWAN technologies could be a bonus goal. In my presentation I had suggested to use the socket address for storing most of that metadata. The counter-proposal was to use netlink for any global setting changes and have the socket just send the actual data. Reminder: We can set SX127x registers before each TX, but RX remained unsolved. For SX130x we can set some metadata per TX (multi-channel), but radio/channel need to have been configured appropriately. Wifi cards may support MIMO but still show up as one wlan0 netdev, so it seemed questionable to diverge for SX130x by having ~10 netdevs and impractical since we have a lot of radio initializations in .ndo_open. Compare slides 16 ff. vs. slide 27: https://events.linuxfoundation.org/wp-content/uploads/2017/12/ELCE2018_LoRa_final_Andreas-Farber.pdf Ultimately the skb is where we need any per-transaction metadata, and outside uapi/ I'm much less concerned about ABI changes. So from my perspective such fields would go into linux/lora/skb.h struct lora_priv after the ifindex, if we go with PF_LORA. As mentioned before, if we have to go with PF_PACKET then I'm clueless where to store such data... > + > +#define LORA_MTU (sizeof(struct lora_frame)) Even if Helmut's comment was mistaken, it doesn't strike me as a good idea to hardcode this here - you could just use data[0] and leave actual MTU to the driver/user. > + > #endif /* _UAPI_LINUX_LORA_H */ Cheers, Andreas
diff --git a/drivers/net/lora/dev.c b/drivers/net/lora/dev.c index 0d4823de8c06..62dbe21668b0 100644 --- a/drivers/net/lora/dev.c +++ b/drivers/net/lora/dev.c @@ -11,8 +11,6 @@ #include <linux/lora/skb.h> #include <net/rtnetlink.h> -#define LORA_MTU 256 /* XXX */ - struct sk_buff *alloc_lora_skb(struct net_device *dev, u8 **data) { struct sk_buff *skb; diff --git a/drivers/net/lora/sx1301.c b/drivers/net/lora/sx1301.c index 9bcbb967f307..fed6d6201bf3 100644 --- a/drivers/net/lora/sx1301.c +++ b/drivers/net/lora/sx1301.c @@ -584,7 +584,7 @@ static int sx1301_load_tx_gain_lut(struct sx1301_priv *priv) return ret; }; -static int sx1301_tx(struct sx1301_priv *priv, void *data, int len) +static int sx1301_tx(struct sx1301_priv *priv, struct lora_frame *frame) { int ret, i; u8 buff[256 + 16]; @@ -617,11 +617,67 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len) hdr->tx_power = 15; /* HACK power entry 15 */ hdr->u.lora.crc16_en = 1; /* Enable CRC16 */ - hdr->u.lora.cr = 1; /* CR 4/5 */ - hdr->u.lora.sf = 7; /* SF7 */ - hdr->u.lora.payload_len = len; /* Set the data len to the skb len */ + + switch (frame->cr) { + case LORA_CR_4_5: + hdr->u.lora.cr = 1; /* CR 4/5 */ + break; + case LORA_CR_4_6: + hdr->u.lora.cr = 2; /* CR 4/6 */ + break; + case LORA_CR_4_7: + hdr->u.lora.cr = 3; /* CR 4/7 */ + break; + case LORA_CR_4_8: + hdr->u.lora.cr = 4; /* CR 4/8 */ + break; + default: + return -ENXIO; + } + + switch (frame->sf) { + case LORA_SF_6: + hdr->u.lora.sf = 6; /* SF6 */ + break; + case LORA_SF_7: + hdr->u.lora.sf = 7; /* SF7 */ + break; + case LORA_SF_8: + hdr->u.lora.sf = 8; /* SF8 */ + break; + case LORA_SF_9: + hdr->u.lora.sf = 9; /* SF9 */ + break; + case LORA_SF_10: + hdr->u.lora.sf = 10; /* SF10 */ + break; + case LORA_SF_11: + hdr->u.lora.sf = 11; /* SF11 */ + break; + case LORA_SF_12: + hdr->u.lora.sf = 12; /* SF12 */ + break; + default: + return -ENXIO; + } + + hdr->u.lora.payload_len = frame->len; /* Set the data length */ hdr->u.lora.implicit_header = 0; /* No implicit header */ - hdr->u.lora.mod_bw = 0; /* Set 125KHz BW */ + + switch (frame->bw) { + case LORA_BW_125KHZ: + hdr->u.lora.mod_bw = 0; /* 125KHz BW */ + break; + case LORA_BW_250KHZ: + hdr->u.lora.mod_bw = 1; /* 250KHz BW */ + break; + case LORA_BW_500KHZ: + hdr->u.lora.mod_bw = 2; /* 500KHz BW */ + break; + default: + return -ENXIO; + } + hdr->u.lora.ppm_offset = 0; /* TODO no ppm offset? */ hdr->u.lora.invert_pol = 0; /* TODO set no inverted polarity */ @@ -632,7 +688,7 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len) /* Copy the TX data into the buffer ready to go */ - memcpy((void *)&buff[16], data, len); + memcpy((void *)&buff[16], frame->data, frame->len); /* Reset any transmissions */ ret = regmap_write(priv->regmap, SX1301_TX_TRIG, 0); @@ -644,7 +700,7 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len) if (ret) return ret; ret = regmap_noinc_write(priv->regmap, SX1301_TX_DATA_BUF_DATA, buff, - len + 16); + frame->len + 16); if (ret) return ret; @@ -653,8 +709,8 @@ static int sx1301_tx(struct sx1301_priv *priv, void *data, int len) if (ret) return ret; - dev_dbg(priv->dev, "Transmitting packet of size %d: ", len); - for (i = 0; i < len + 16; i++) + dev_dbg(priv->dev, "Transmitting packet of size %d: ", frame->len); + for (i = 0; i < frame->len + 16; i++) dev_dbg(priv->dev, "%X", buff[i]); return ret; @@ -683,17 +739,18 @@ static void sx1301_tx_work_handler(struct work_struct *ws) struct sx1301_priv *priv = container_of(ws, struct sx1301_priv, tx_work); struct net_device *netdev = dev_get_drvdata(priv->dev); + struct lora_frame *frame = (struct lora_frame *)priv->tx_skb->data; int ret; netdev_dbg(netdev, "%s\n", __func__); if (priv->tx_skb) { - ret = sx1301_tx(priv, priv->tx_skb->data, priv->tx_skb->len); + ret = sx1301_tx(priv, frame); if (ret) { netdev->stats.tx_errors++; } else { netdev->stats.tx_packets++; - netdev->stats.tx_bytes += priv->tx_skb->len; + netdev->stats.tx_bytes += frame->len; } if (!(netdev->flags & IFF_ECHO) || diff --git a/include/uapi/linux/lora.h b/include/uapi/linux/lora.h index 4ff00b9c3c20..d675025d2a6e 100644 --- a/include/uapi/linux/lora.h +++ b/include/uapi/linux/lora.h @@ -21,4 +21,50 @@ struct sockaddr_lora { } lora_addr; }; +#define LORA_MAX_DLEN 256 + +enum lora_bw { + LORA_BW_125KHZ, + LORA_BW_250KHZ, + LORA_BW_500KHZ, +}; + +enum lora_cr { + LORA_CR_4_5, + LORA_CR_4_6, + LORA_CR_4_7, + LORA_CR_4_8, +}; + +enum lora_sf { + LORA_SF_6, + LORA_SF_7, + LORA_SF_8, + LORA_SF_9, + LORA_SF_10, + LORA_SF_11, + LORA_SF_12, +}; + +/** + * struct lora_frame - LoRa frame structure + * @freq: Frequency of LoRa transmission in Hz + * @power: Power of transmission in dBm + * @bw: bandwidth, 125, 250 or 500 KHz + * @cr: coding rate, 4/5 to 4/8 + * @sf: spreading factor from SF6 to 12 + * @data: LoRa frame payload (up to LORA_MAX_DLEN byte) + */ +struct lora_frame { + __u32 freq; /* transmission frequency in Hz */ + __u8 power; /* transmission power in dBm */ + enum lora_bw bw; /* bandwidth */ + enum lora_cr cr; /* coding rate */ + enum lora_sf sf; /* spreading factor */ + __u8 len; + __u8 data[LORA_MAX_DLEN] __attribute__((aligned(8))); +}; + +#define LORA_MTU (sizeof(struct lora_frame)) + #endif /* _UAPI_LINUX_LORA_H */
Information such as spreading factor, coding rate and power are on a per transmission basis so it makes sence to include this in a header much like CAN frames do. Signed-off-by: Ben Whitten <ben.whitten@lairdtech.com> --- drivers/net/lora/dev.c | 2 - drivers/net/lora/sx1301.c | 79 +++++++++++++++++++++++++++++++++------ include/uapi/linux/lora.h | 46 +++++++++++++++++++++++ 3 files changed, 114 insertions(+), 13 deletions(-)