Comments (10)
Attached is the proposed fix.
This fix closes all holes that could cause erroneous endPacket signals from
being generated. startPacket is only issued on the transition to SEQNO and
endPacket is only signalled if the state machine is in either SEQNO or INFO.
This code has been tested in its corner cases using gdb/jtag on a msp430f2618
based mote.
SerialP needs to be used with the modified SerialDispatchP that is part of
Issue 2. Both files need to be replaced to fix bugs in both that can cause
packet lossage.
Original comment by [email protected]
on 20 Oct 2010 at 10:02
Attachments:
from tinyos-main.
[deleted comment]
from tinyos-main.
Generally, it's bad to use enums as a type (e.g. rxstate_t). The C compiler
defaults enums to ints; if the int is 16 bits, you waste a byte. These are the
kinds of little stylistic changes, which, when mixed in with functional
changes, lead code to bitrot.
Same as the reversal of the RXSTATE_NOSYNC case. Is there a functional problem
here, or a stylistic one? The difference seems to be return vs. break. Why
reverse the statement? Was there a problem?
The RXSTATE_PROTO has a similar reversal, although now it looks functionally
different. Generally, the entire receive switch has been rewritten. It's not
clear which of the changes are intended to fix bugs 1 and 2, and which are just
stylistic. I'd feel much more comfortable if we had a version which fixes the
bugs, then one which stylistically refactors the code. In the former, it's
possible to through thought experiments and testing to determine if the bug is
fixed. In the latter, it's possible through thought experiments and testing to
determine if the code is equivalent. Since the two are mixed, it's very hard to
do either.
Why is #2 a bug? Is the desired protocol that endPacket is only paired on a
successful startPacket? The text above seems to suggest that this should be the
case, as do the interface comments. But #2 suggests there shouldn't be. Which
is right?
#1 seems like a definite bug, which requires only a tiny change (the case in
RXSTATE_PROTO not going to nosync but rather resetting the state machine). Why
is this not sufficient?
Original comment by [email protected]
on 27 Oct 2010 at 9:50
from tinyos-main.
With respect to the use of ENUMs:
It is not stylistic sugar but rather an issue of trade offs.
On the MSP430 the toolchain allocates the native size which is 16 bits. The
trade off is code vs. ram.
If an ENUM is used it uses an extra byte of RAM but generates better code.
Using an uint8_t on the other hand saves the byte of RAM but everywhere the
object is referenced extra code is generated that maps the 8 bit datum to the
underlying machine size. (in this case 16 bits).
So the question is where does one want to burn the memory, code or data? 1
extra byte of RAM vs. multiple code bytes.
An associated impact is how does the object interact with symbolic debuggers
and debugging this code. The majority of time getting code working properly is
debugging and symbolic debuggers greatly aid in that. Using enums works better
with the debugger and is symbolic while uint8_t is just a number.
Original comment by [email protected]
on 28 Oct 2010 at 5:03
from tinyos-main.
Same as the reversal of the RXSTATE_NOSYNC case. Is there a functional problem
here, or a stylistic one? The difference seems to be return vs. break. Why
reverse the statement? Was there a problem?
----------------
The rx_state_machine was rewritten to make it simpler and easier to understand.
Being easy to understand and clean to look at makes it much more likely that
the state machine will do the right thing. If it is easy to look at and
understand it should be easy to see its correctness. That was the thinking
anyway.
States that are complete immediately return as opposed to breaking and then
flowing through the end of the code either via the break or a goto. States
that needed to abort would do so via a break and the abort code immediately
followed the switch.
This seems to be much cleaner, easier to understand. Okay subjective.
The reversal of the RXSTATE_NOSYNC case is stylistic. Minimize indent coupled
with early processing if possible.
Original comment by [email protected]
on 28 Oct 2010 at 5:47
from tinyos-main.
1) Invalid serial protocol. The first byte following the HDLC start of packet
delimiter is the low level serial protocol. Anything other than
SERIAL_PROTO_PACKET_ACK will cause an endPACKET(FAIL) to be signalled.
---
#1 seems like a definite bug, which requires only a tiny change (the case in
RXSTATE_PROTO not going to nosync but rather resetting the state machine). Why
is this not sufficient?
+++++++++++++++
The intent of the label "nosync:" and the goto nosync is to reset the state
machine. The label "nosync" is a place in the code that forces the rx state
machine to the NOSYNC state while also resetting whatever is necessary to fully
reset the machine. Counters and other state variables. It also tells the
underlying layer to reset.
So the intent is indeed to reset the state machine in a well defined common
manner.
One could set the rx_state to RXSTATE_NOSYNC but this special cases things
without clearing out underlying state variables. Is that what you meant by
"resetting the state machine"?
Seemed like having a consistent mechanism for "processing complete" and for
"abort" was a good idea.
+++++++++++++++
Due to problems with SerialP's implementation, solitary endPackets can be
generated. This can occur in the following cases:
2) If startPacket() (the upper layer signal) returns FALSE, endPACKET(FAIL) is
signalled.
---
Why is #2 a bug? Is the desired protocol that endPacket is only paired on a
successful startPacket? The text above seems to suggest that this should be the
case, as do the interface comments. But #2 suggests there shouldn't be. Which
is right?
+++++++++++++++
Yes, endPacket should only be allowed to follow an startPacket that returns
SUCCESS. #2 violates this and is a bug. The endPacket(FAIL) is signalled
and is a bug.
Premise is startPacket returns FALSE, ==> upper layer buffer is locked. Then
SerialP signals endPacket(FAIL), this causes the current buffer to be unlocked
and made available for a future receive.
Because this buffer was originally locked, it was handed off to the
receiveTask. The unlock violates mutual exclusion of the packet data.
That is why #2 is a bug. Is there some interaction with SerialDispatcherP such
that the premise would be invalid?
Original comment by [email protected]
on 28 Oct 2010 at 6:31
from tinyos-main.
[deleted comment]
from tinyos-main.
Can we close on this? Eric, your most recent emails suggest that fixing the
start/end problem is sufficient.
Original comment by [email protected]
on 13 Nov 2010 at 1:12
- Changed state: Started
from tinyos-main.
This issue can be closed. The analysis of the problem involved a premise that
isn't true:
"This problem will only cause a failure iff there are two buffers at the
SerialDispatcherP layer waiting to be delivered."
SerialDispatcherP does not allow for two packets to be pending at the same time
rendering the analysis moot.
Original comment by [email protected]
on 13 Nov 2010 at 1:42
from tinyos-main.
Original comment by [email protected]
on 13 Nov 2010 at 2:22
- Changed state: Fixed
from tinyos-main.
Related Issues (20)
- [patch] UdpP scribbles to memory it should not. HOT 3
- [patch] IPPacketC uses error return value as offset, resulting in trashed memory HOT 3
- "Make safe" does not work with the CC2420 radio chip HOT 2
- Msp430Adc12ClientAutoRVGC does not release resource. HOT 2
- PowerCycleP__startRadio task in permanent re-post spin HOT 2
- Memory Leaks : When check out some unknow critical option Libcoap allocate memory but never free HOT 3
- TOSSIM PYTHON_VERSION on Ubuntu 11.04 x86_64 HOT 3
- Shimmer I2C with msp430-gcc v4.5.3 HOT 3
- Shimmer PowerSupplyMonitor with msp430-gcc v4.5.3 HOT 2
- msp430 code accessed with bit fields HOT 1
- tos-bsl broken for shimmer family HOT 6
- [patch] CoapUdpServer infinite loop HOT 4
- [patch] CoapUdpServer fails to include the client token in many cases HOT 3
- telosb CC2420X compile error with LPL app HOT 1
- [patch] Incorrect 6lowpan PAN handling HOT 4
- rfxlink TrafficMonitorLayerP time variable shadows global one when compiling with TOSSIM HOT 3
- [patch] RF212 frame receive doesn't always complete SPI transfer HOT 2
- [patch] Dhcp6Client fails to find Status options HOT 2
- [patch] Dhcp6Client issues IPAddress.setAddress() even on RENEW HOT 1
- [patch] Dhcp6Client waits unnecessarily long between ADVERTISE and REQUEST
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from tinyos-main.