-
Notifications
You must be signed in to change notification settings - Fork 2k
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
at86rf2xx: introduce pending TX counter #5261
Conversation
at86rf2xx_set_state(dev, dev->idle_state); | ||
/* check for more pending TX calls and return to idle state if | ||
* there are none */ | ||
if (--dev->pending_TX <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unsigned variable is unlikely to be less than 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smartass! ;-b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
72e0bab
to
33612c7
Compare
@@ -163,6 +163,9 @@ typedef struct { | |||
uint8_t page; /**< currently used channel page */ | |||
#endif | |||
uint8_t idle_state; /**< state to return to after sending */ | |||
uint8_t pending_TX; /**< keep track of pending TX calls | |||
this is required to known when to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/known/know/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and please TX->tx
This also fixes #4725. Thanks Oleg for looking into it! I would ACK but maybe @authmillenon should take another look? |
I tried to bend my head around this but can't see the situation described above. |
@thomaseichinger, the state is reset to RX in More understandable? |
Addressed comments. Btw. thanks to @haukepetersen for his hints while debugging. |
Aah! |
Yes, I know, it's a bit confusing that a function named ISR is not called in interrupt context. ;) |
It is, we might should consider renaming it for clarity. Despite of this, ugliness aside, I like this change because it massively saves time we would poll against state changes. |
I second @kaspar030's ACK. |
wait a second - reading @haukepetersen's thoughts in #5257 I wonder what happens if the message queue is too small (i.e. too many sending calls arrive too quickly). Wouldn't we fail to handle the isr in that case? |
I think not, as we still actively block in |
Hm, yes, I guess you're right. |
Should I squash then? |
let's see, two possible orders:
So it seems to me, that we always actively block the thread before we can overfill the msg queue. |
yes, please |
At least if our queue can hold more than one packet. |
This counter is necessary for the current concept to tell the driver when to return to idle after sending.
1245ff5
to
252baec
Compare
squashed |
true :-) So our current setting with 8 packets should be sufficient |
Apparently |
This PR made lwIP not able to receive anymore (the interrupt on receive isn't even fired). Any idea why? |
This is another (better) workaround to solve #5257 compared to #5258.
The problem is that one might want to send several packets back to back. The netdev2 event loop will now queue these sending requests in the message queue. Once the first TX is completed it will queue another event in the queue. Once this is handled it would reset the state to idle (RX) again. Now the next TX completed event is handled and interpret the IRQ as an RX event, because it was received in RX state.
This PR counts the currently pending TX calls to only return to idle (RX) after all TX calls are handled.
A better solution for this would be to modify the driver to use thread_flags instead of msg, but if we cannot find the time to include this for the release, I would propose to take this intermediate solution.