Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/at86rf2xx: Add delay for at86rf212b driver from sleep to wake up #7865

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

biboc
Copy link
Member

@biboc biboc commented Oct 26, 2017

Wake up from sleep mode for at86rf212B fails most of the time because delay is for at86rf233 so I added right delay for at86rf212b

@biboc biboc added the Area: drivers Area: Device drivers label Oct 26, 2017
@jnohlgard
Copy link
Member

Both the rf212b and rf233 data sheets say TR1a is max 1000 us. The typical values are different though. Perhaps it is safest to just go with the worst case timing for both if we don't have any other way of checking that the oscillator is ready?

@alexaring
Copy link

We use 1000us on all chips in the Linux kernel. See:

http://elixir.free-electrons.com/linux/v4.14-rc6/source/drivers/net/ieee802154/at86rf230.c#L1354

So far no problems reported.

@biboc
Copy link
Member Author

biboc commented Oct 26, 2017

Yes this is why I took 1100us to be 100% sure that driver can use transceiver

*/
#ifdef MODULE_AT86RF212B
#define AT86RF2XX_WAKEUP_DELAY (1100U)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 1000 should be enough, you'll get a margin anyway because of processing delays before and after the 1000 us wait.

@jnohlgard
Copy link
Member

I recommend that we set this to 1000 on all chips for now. The proper fix IMO would be to enable the AWAKE_END interrupt before sleeping the transceiver and waiting for that instead of sleeping for X µs.

@biboc
Copy link
Member Author

biboc commented Oct 31, 2017

Yes we should wait IRQ4 (AWAKE_END) I agree but how?
Since _isr run in same thread at86rf2xx_assert_awake, at86rf2xx_assert_awake can't wait _isr AT86RF2XX_IRQ_STATUS_MASK__CCA_ED_DONE is called
What do you think?

BTW, I still have problem waken up the transceiver (with DELAY 1100), when this happen, the transceiver can send a packet but don't receive the answer

@alexaring
Copy link

In Linux I would do something for awake: ENABLE_IRQ -> $SLP_TR_PIN -> "wait_for_completion(foo)" and isr for AWAKE_END makes complete(foo); - RIOT_OS should have similar paradigms?

Also if you are in sleep mode do you disable irq? Be also aware that you cannot do register settings while transceiver is in sleep.

@biboc
Copy link
Member Author

biboc commented Nov 1, 2017

@alexaring, I'm not sure I understood your paradigm
The function wait_for_completion(foo) will wait for a flag, won't it?
And then ISR is called and flag is changed with complete(foo)?

In RIOT, an ISR for transceiver send a message to at86rf2xx driver to run _isr() function where AWAKE_END is parsed. But this message will be executed after wait_for_completion() ends so it's going to never happen. Do you see what I mean?

@alexaring
Copy link

Why you send a message to the transceiver when you wait for AWAKE_END? The transceiver should be in sleep state. There should nothing send to the transceiver in sleep state.

@biboc
Copy link
Member Author

biboc commented Nov 2, 2017

@alexaring, Have you look at how transceiver implementation work in RIOT?
Here is the irq called when transceiver trigger the IRQ:
https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx_netdev.c#L59
This sends a message to at86rf2xx thread:
https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/link_layer/netdev/gnrc_netdev.c#L59
Then at86rf2xx thread can manage the isr:
https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx_netdev.c#L563

So if the thread is executing a command GO_TO_SLEEP and waits for the FLAG, the _isr will never be called.
Do you see what I mean?

@biboc
Copy link
Member Author

biboc commented Nov 2, 2017

Anyway, I found my problem, I'll push a fix
Going from TX_ARET_ON to TRX_OFF fails so moving to idle_state before solves the problem

@alexaring
Copy link

So if the thread is executing a command GO_TO_SLEEP and waits for the FLAG, the _isr will never be called.

You should be sure that there is no command anymore which waits for something before GO_TO_SLEEP.

If transceiver is sending a message while we want to set transceiver to sleep, go to idle state then TRX_OFF then SLEEP.
@biboc
Copy link
Member Author

biboc commented Nov 2, 2017

I pushed the fix and change 1100us by 1000

@biboc
Copy link
Member Author

biboc commented Nov 2, 2017

@alexaring
Copy link

alexaring commented Nov 2, 2017

https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx_getset.c#L473

This is racy the state can be changed from RX_AACK_ON to BUSY_RX_AACK automatically (by transceiver) after the loop.

Means this check cannot be sure that after the loop you are not in BUSY_RX_AACK.

@biboc
Copy link
Member Author

biboc commented Nov 2, 2017

Yes I agree, you do you do in Linux?

@alexaring
Copy link

Yes, I maintain the driver in Linux and know some pitfalls of this transceiver.

@biboc
Copy link
Member Author

biboc commented Nov 3, 2017

Sorry, How do you do in Linux?

@alexaring
Copy link

In Linux we do sleep only if you ifdown all interfaces which operating on the PHY. If all interfaces are down, the PHY isn't used and will go into sleep. You can access register settings for MAC/PHY parameters while ifdown, but no transmission can be initiated, only if you do ifup on one interface. Then the transceiver is not anymore in sleep state.

Not sure if this answer helps you.

@jnohlgard
Copy link
Member

I posted an implementation which uses the AWAKE_END IRQ to signal exit from sleep, PR #8230.

Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline question

/* When on TX_ARET_ON, go to idle state before going to TRX_OFF */
assert(dev->pending_tx != 0);
if ((--dev->pending_tx) == 0) {
at86rf2xx_set_state(dev, dev->idle_state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My interpretation of the data sheet is that the FORCE_TRX_OFF state should be used to abort any ongoing transfers in the middle of everything and move the transceiver to TRX_OFF to ensure that the state machine is in a known state before changing settings or modes. I think this change will add unexpected waits for TX completion in places where no wait is expected. Perhaps it is the usage of FORCE_TRX_OFF which is wrong? What were the problems that you were seeing in your application? Did the pending_tx counter get stuck at 1 and never dropping back to 0?
Perhaps we could just reset that counter instead of waiting for TX completion?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this because transition from AT86RF2XX_STATE_TX_ARET_ON to AT86RF2XX_STATE_SLEEP failed. It's not a common case because most of the time, transceiver is in IDLE state before going to SLEEP state.
When I say it fails, it means that transceiver does not go to sleep and then when we wake it up, it doesn't.

Each time transceiver succeed to send a frame, it raises an IRQ and the driver changes the state of the transceiver to idle_state before continue the execution.
So i did the same outside the IRQ which works for me
You can see what driver does in:
static void _isr(netdev_t *netdev)

I didn't check if pending_tx counter gets stuck

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you suggest?

@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@PeterKietzmann PeterKietzmann added the State: don't stale State: Tell state-bot to ignore this issue label Aug 19, 2019
@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 19, 2019
@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers State: don't stale State: Tell state-bot to ignore this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants