Hello.
On Wed, 2013-03-13 at 11:38, Alan Ott wrote:
> On 03/13/2013 03:45 AM, Wolf-Bastian Pöttner wrote:
> > Implement the filter function to update short address, pan id and ieee
> > address on change. Allowing for hardware address filtering needed for
> > auto ACK. Switch state machine to use auto ACK states for rx.
> >
> > This will handle CSMA/CA for us as well as re-sending frames when no ack
> > comes
> > in timely.
> >
> > The register names have been wrong since the beginning but it only showed
> > up now
> > as they are actualy used for the upcoming auto ACK support.
> >
> > Signed-off-by: Stefan Schmidt <ste...@datenfreihafen.org>
> > ---
> > drivers/ieee802154/at86rf230.c | 108
> > ++++++++++++++++++++++++++++++++++++++-
> > 1 files changed, 105 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/ieee802154/at86rf230.c b/drivers/ieee802154/at86rf230.c
> > index 4d32ac4..761343b 100644
> > --- a/drivers/ieee802154/at86rf230.c
> > +++ b/drivers/ieee802154/at86rf230.c
> > @@ -38,6 +38,9 @@
> > long unsigned int bytes_tx;
> > long unsigned int bytes_rx;
> >
> > +#define ENABLE_AACK
> > +#define ENABLE_ARET
> > +
>
> #ifdef stuff isn't going to fly in the mainline kernel. Just a heads-up.
Yeah, well aware of that. It was a shortcut for me as the stack did
not provide any means to hook configuration up for this.
> Ack support is something that's been on my radar to add, but haven't had
> time. Maybe you have time to do this properly, by making a new netlink
> item to turn acking on and off and pushing it through iz.
I don't think I will have the time for this. Its sad but I have to be
realistic here. I did all that during my diploma thesis which is done
over a year now and I started to work full-time since and moved
countries.
I would be happy to adapt this patch once the stack offers such
functionality though.
> > struct at86rf230_local {
> > struct spi_device *spi;
> > int rstn, slp_tr, dig2;
> > @@ -236,8 +239,8 @@ struct at86rf230_local {
> > #define STATE_SLEEP 0x0F
> > #define STATE_BUSY_RX_AACK 0x11
> > #define STATE_BUSY_TX_ARET 0x12
> > -#define STATE_BUSY_RX_AACK_ON 0x16
> > -#define STATE_BUSY_TX_ARET_ON 0x19
> > +#define STATE_RX_AACK_ON 0x16
> > +#define STATE_TX_ARET_ON 0x19
> > #define STATE_RX_ON_NOCLK 0x1C
> > #define STATE_RX_AACK_ON_NOCLK 0x1D
> > #define STATE_BUSY_RX_AACK_NOCLK 0x1E
> > @@ -462,7 +465,11 @@ at86rf230_state(struct ieee802154_dev *dev, int state)
> > might_sleep();
> >
> > if (state == STATE_FORCE_TX_ON)
> > +#ifdef ENABLE_ARET
> > + desired_status = STATE_TX_ARET_ON;
> > +#else
> > desired_status = STATE_TX_ON;
> > +#endif
> > else if (state == STATE_FORCE_TRX_OFF)
> > desired_status = STATE_TRX_OFF;
> > else
> > @@ -488,10 +495,43 @@ at86rf230_state(struct ieee802154_dev *dev, int state)
> > goto err;
> > } while (val == STATE_TRANSITION_IN_PROGRESS);
> >
> > +#ifdef ENABLE_ARET
> > + /* Make sure we go to TX_ON before we go to STATE_TX_ARET_ON */
> > + if (desired_status == STATE_TX_ARET_ON) {
> > + rc = at86rf230_write_subreg(lp, SR_TRX_CMD, STATE_TX_ON);
> > + if (rc)
> > + goto err;
> > +
> > + do {
> > + rc = at86rf230_read_subreg(lp, SR_TRX_STATUS, &val);
> > + if (rc)
> > + goto err;
> > + pr_debug("%s val3 = %x\n", __func__, val);
> > + } while (val == STATE_TRANSITION_IN_PROGRESS);
> > +
> > + rc = at86rf230_write_subreg(lp, SR_TRX_CMD, desired_status);
> > + if (rc)
> > + goto err;
> > +
> > + do {
> > + rc = at86rf230_read_subreg(lp, SR_TRX_STATUS, &val);
> > + if (rc)
> > + goto err;
> > + pr_debug("%s val4 = %x\n", __func__, val);
> > + } while (val == STATE_TRANSITION_IN_PROGRESS);
> > + }
> > +#endif
> >
> > if (val == desired_status)
> > return 0;
> >
> > +#ifdef ENABLE_AACK
> > + if (state == STATE_RX_AACK_ON && val == STATE_BUSY_RX_AACK)
> > +#else
> > + if (state == STATE_RX_ON && val == STATE_BUSY_RX)
> > +#endif
> > + return 0;
> > +
> > pr_err("unexpected state change: %d, asked for %d\n", val, state);
> > return -EBUSY;
> >
> > @@ -510,7 +550,11 @@ at86rf230_start(struct ieee802154_dev *dev)
> > if (rc)
> > return rc;
> >
> > +#ifdef ENABLE_AACK
> > + return at86rf230_state(dev, STATE_RX_AACK_ON);
> > +#else
> > return at86rf230_state(dev, STATE_RX_ON);
> > +#endif
> > }
> >
> > static void
> > @@ -625,6 +669,54 @@ err:
> > return -EINVAL;
> > }
> >
> > +#ifdef ENABLE_AACK
> > +static int
> > +at86rf230_set_hw_addr_filt(struct ieee802154_dev *dev,
> > + struct ieee802154_hw_addr_filt
> > *filt,
> > + unsigned long changed)
> > +{
> > + struct at86rf230_local *lp = dev->priv;
> > +
> > + might_sleep();
> > +
> > + at86rf230_stop(dev);
> > +
> > + msleep(10);
>
> Why this msleep()? is this from the datasheet, or is it a hack? Also,
> you might get pushback on using a magic number (10) there, and not a
> #define.
To be honest I can't remember. Would need to check the datasheet.
> > +
> > + if (changed & IEEE802515_AFILT_SADDR_CHANGED) {
> > + __at86rf230_write(lp, RG_SHORT_ADDR_0, filt->short_addr &
> > 0xff); /* LSB */
> > + __at86rf230_write(lp, RG_SHORT_ADDR_1, (filt->short_addr >> 8)
> > & 0xff); /* MSB */
> > + }
> > +
> > + if (changed & IEEE802515_AFILT_PANID_CHANGED) {
> > + __at86rf230_write(lp, RG_PAN_ID_0, filt->pan_id & 0xff); /* LSB
> > */
> > + __at86rf230_write(lp, RG_PAN_ID_1, (filt->pan_id >> 8) & 0xff);
> > /* MSB */
> > + }
> > +
> > + if (changed & IEEE802515_AFILT_IEEEADDR_CHANGED) {
> > + // Make sure order MSB to LSB is correct
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_0, filt->ieee_addr[7]);
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_1, filt->ieee_addr[6]);
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_2, filt->ieee_addr[5]);
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_3, filt->ieee_addr[4]);
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_4, filt->ieee_addr[3]);
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_5, filt->ieee_addr[2]);
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_6, filt->ieee_addr[1]);
> > + at86rf230_write_subreg(lp, SR_IEEE_ADDR_7, filt->ieee_addr[0]);
> > + }
> > +
> > + if (changed & IEEE802515_AFILT_PANC_CHANGED) {
> > + if (filt->pan_coord)
> > + at86rf230_write_subreg(lp, SR_AACK_I_AM_COORD, 1);
> > + else
> > + at86rf230_write_subreg(lp, SR_AACK_I_AM_COORD, 0);
> > + }
> > +
> > + at86rf230_start(dev);
> > +
> > + return 0;
> > +}
> > +#endif
>
> We need filter stuff regardless of acking, so split this off into a
> separate patch.
You mena a separate patch that only introduces the address filtering
to at86rf230 and hooks it up to the core? Splitting this out should be
easy. The way more time consuming part for me will be getting myself a
test setup again. It would include forward porting my atus driver
which uses at86rf230 and is the only hardware I have.
I put this on my todo for the weekend and will spent some time on it.
Can't promise anything though.
> I do question the need to _start() and _stop() though. What makes you
> think this is necessary?
Can't remember either if this was a shortcut or not.
> > static struct ieee802154_ops at86rf230_ops = {
> > .owner = THIS_MODULE,
> > .xmit = at86rf230_xmit,
> > @@ -632,6 +724,9 @@ static struct ieee802154_ops at86rf230_ops = {
> > .set_channel = at86rf230_channel,
> > .start = at86rf230_start,
> > .stop = at86rf230_stop,
> > +#ifdef ENABLE_AACK
> > + .set_hw_addr_filt = at86rf230_set_hw_addr_filt,
> > +#endif
> > };
> >
> > static void at86rf230_irqwork(struct work_struct *work)
> > @@ -680,7 +775,6 @@ static irqreturn_t at86rf230_isr(int irq, void *data)
> > return IRQ_HANDLED;
> > }
> >
> > -
>
> ^^^ Stray whitespace change
>
> > static int at86rf230_hw_init(struct at86rf230_local *lp)
> > {
> > u8 status;
> > @@ -718,7 +812,11 @@ static int at86rf230_hw_init(struct at86rf230_local
> > *lp)
> > /* Wait the next SLEEP cycle */
> > msleep(100);
> >
> > +#ifdef ENABLE_ARET
> > + rc = at86rf230_write_subreg(lp, SR_TRX_CMD, STATE_TX_ARET_ON);
> > +#else
> > rc = at86rf230_write_subreg(lp, SR_TRX_CMD, STATE_TX_ON);
> > +#endif
> > if (rc)
> > return rc;
> > msleep(1);
> > @@ -802,7 +900,11 @@ static int __devinit at86rf230_probe(struct spi_device
> > *spi)
> > dev->extra_tx_headroom = 0;
> > /* We do support only 2.4 Ghz */
> > dev->phy->channels_supported[0] = 0x7FFF800;
> > +#ifdef ENABLE_AACK
> > + dev->flags = IEEE802154_HW_OMIT_CKSUM | IEEE802154_HW_AACK;
> > +#else
> > dev->flags = IEEE802154_HW_OMIT_CKSUM;
> > +#endif
> >
> > mutex_init(&lp->bmux);
> > INIT_WORK(&lp->irqwork, at86rf230_irqwork);
>
> It looks like you've put some work into this. Good job. I think a little
> bit more and it'll be first class. :)
Thanks. I will see if I can find the time and motivation to split the
address filtering out and update my atusb driver in the process (also
missing from mainline).
The changes to the stack to allow auto ack configuration is nothing I
want to put on my list and never deliver.
I'm not sure if Basti pushed all the patches or only the onces he
finds worthwhile. Seems I need to dig up all the stuff. :)
Basti, if you read this. What is about the patch from Stephen to fix
the address in the recvfrom()? I have seen a patch but I'm uncertain
if it ever hit mainline.
regards
Stefan Schmidt
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_mar
_______________________________________________
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
