Hi all, I reviewed the wifi mesh code, all comments and reviews are welcome. Best regards Faker Moatamri http://codereview.appspot.com/63188/diff/1/2 File examples/mesh.cc (right): http://codereview.appspot.com/63188/diff/1/2#newcode70 Line 70: /// Create nodes and setup theis mobility spelling: their http://codereview.appspot.com/63188/diff/1/3 File src/devices/mesh/dot11s/airtime-metric.cc (right): http://codereview.appspot.com/63188/diff/1/3#newcode60 Line 60: AirtimeLinkMetricCalculator::CalculateMetric(Mac48Address peerAddress, Ptr<MeshWifiInterfaceMac> mac) What kind of metric are you calculating here? some comments will be welcome like those in the header file. http://codereview.appspot.com/63188/diff/1/3#newcode70 Line 70: uint32_t metric = (uint32_t) ( What is the 10240 number? or 8* 1e9? is it possible to use a constant in the standard form instead of using numbers? http://codereview.appspot.com/63188/diff/1/5 File src/devices/mesh/dot11s/dot11s-helper.cc (right): http://codereview.appspot.com/63188/diff/1/5#newcode73 Line 73: if (channel > 0) Kill commented code... http://codereview.appspot.com/63188/diff/1/5#newcode132 Line 132: //if(*root_iterator == i.GetDistanceFrom(c.Begin ())) kill commented code http://codereview.appspot.com/63188/diff/1/5#newcode144 Line 144: return Install (phy, NodeContainer (node), roots, nInterfaces); Here you are iterating on a list of one node. Is it possible to implement the Install for one node and then to call it from the iterator in the above function: for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i) { Ptr<Node> node = *i; Install(...,node,...) } I think it might make more sense to do it this way... http://codereview.appspot.com/63188/diff/1/6 File src/devices/mesh/dot11s/dot11s-helper.h (right): http://codereview.appspot.com/63188/diff/1/6#newcode57 Line 57: * \brief Install 802.11s mesh device & protocols on given node This installs on given node list right? http://codereview.appspot.com/63188/diff/1/7 File src/devices/mesh/dot11s/dot11s-mac-header.cc (right): http://codereview.appspot.com/63188/diff/1/7#newcode113 Line 113: return; May be should give here a warning or throw an error that we cannot have more than 3 addresses? http://codereview.appspot.com/63188/diff/1/7#newcode256 Line 256: NS_ASSERT (false); This will generate an assertion fault, the return above is useless since it will stop anyway. I suggest putting output error since I guess this is an error (is it?), it would make more sense to generate an error here, but if you really want to keep NS_ASSERT, please output some text describing the error. http://codereview.appspot.com/63188/diff/1/7#newcode275 Line 275: NS_ASSERT (false); Same comment http://codereview.appspot.com/63188/diff/1/7#newcode279 Line 279: // ??? If this code needs to be put, please put a TODO to be clear that is missing code. If the behavior is not specified yet, please put a comment on that. http://codereview.appspot.com/63188/diff/1/7#newcode286 Line 286: return retval; same comment http://codereview.appspot.com/63188/diff/1/7#newcode330 Line 330: { did you mean Dot11sMacHeaderBits? http://codereview.appspot.com/63188/diff/1/7#newcode340 Line 340: bool result (true); Testing the serialization would be very recommended here to avoid unexpected behavior. http://codereview.appspot.com/63188/diff/1/8 File src/devices/mesh/dot11s/dot11s-mac-header.h (right): http://codereview.appspot.com/63188/diff/1/8#newcode46 Line 46: void SetAddr4 (Mac48Address address); What are those addresses? why 4,5,6... not 1,2,3...? http://codereview.appspot.com/63188/diff/1/10 File src/devices/mesh/dot11s/hwmp-mac-plugin.cc (right): http://codereview.appspot.com/63188/diff/1/10#newcode52 Line 52: { In this function, I would recommand creating two private functions that will treat the mesh header and the action respectively. This will increase readability specially if you plan to add more functionalities in here. http://codereview.appspot.com/63188/diff/1/10#newcode75 Line 75: NS_ASSERT(false); Message saying that extension is not supported yet would be appreciated. Same for all the NS_ASSERT(false) in your code. http://codereview.appspot.com/63188/diff/1/10#newcode81 Line 81: return false; your return will never be called right? http://codereview.appspot.com/63188/diff/1/10#newcode196 Line 196: NS_ASSERT (!m_preqTimer.IsRunning()); This line and the if (m_preqTimer.IsRunning ()) return; are doing the same job right? http://codereview.appspot.com/63188/diff/1/11 File src/devices/mesh/dot11s/hwmp-mac-plugin.h (right): http://codereview.appspot.com/63188/diff/1/11#newcode67 Line 67: //add a destination to exisying PREQ generated by me and stored in existing, may be use the /** * */ http://codereview.appspot.com/63188/diff/1/12 File src/devices/mesh/dot11s/hwmp-protocol.cc (right): http://codereview.appspot.com/63188/diff/1/12#newcode193 Line 193: HwmpProtocol::RequestRoute ( This functionality might be given to another class to make the protocol class less heavy, and it would be called by the protocol as a service. http://codereview.appspot.com/63188/diff/1/12#newcode208 Line 208: NS_ASSERT (false); Again the NS_ASSERT(false)... please replace all of them in your code by NS_ERROR and please write a user friendly message to explain the error or the not yet supported feature. http://codereview.appspot.com/63188/diff/1/12#newcode220 Line 220: return false; again return never reached... http://codereview.appspot.com/63188/diff/1/12#newcode239 Line 239: if(*chan == plugin->second->GetChannelId ()) for a matter of clarity, you should may be use brackets here to make it more readable. http://codereview.appspot.com/63188/diff/1/12#newcode472 Line 472: //chack if must retransmit: check http://codereview.appspot.com/63188/diff/1/12#newcode733 Line 733: retval.pkt = NULL; Please use 0 instead, please refer to coding style. http://codereview.appspot.com/63188/diff/1/12#newcode764 Line 764: while (1) why not put while (packet.pkt != 0) instead of the infinite loop? http://codereview.appspot.com/63188/diff/1/12#newcode767 Line 767: if (packet.pkt == NULL) 0 instead of NULL http://codereview.appspot.com/63188/diff/1/12#newcode833 Line 833: while (1) same as above http://codereview.appspot.com/63188/diff/1/13 File src/devices/mesh/dot11s/hwmp-protocol.h (right): http://codereview.appspot.com/63188/diff/1/13#newcode101 Line 101: ///\brief forms a path error information element when list of destination fails on a given interface This class is too huge, it should delegate some responsibilities to other classes, it will be more readable and more maintainable and eventually reusable. For example Path error handling can be in a separate class. Same thing for queuing and dequeuing,stats... Always try to make the classes sepcialized and their functionalities limited to ease, readibility, maintainability, extensibility and reusability. http://codereview.appspot.com/63188/diff/1/14 File src/devices/mesh/dot11s/hwmp-rtable.cc (right): http://codereview.appspot.com/63188/diff/1/14#newcode230 Line 230: //We suppose that no dublicates here can be duplicates http://codereview.appspot.com/63188/diff/1/14#newcode311 Line 311: void HwmpRtableTest::Test1 () Please give a more signicative name to the tests you are executing, in order to give an idea of what you are doing or add some comments about it. http://codereview.appspot.com/63188/diff/1/14#newcode347 Line 347: // TODO: test AddPrecursor and GetPrecursors will you be able to do it soon? http://codereview.appspot.com/63188/diff/1/18 File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.cc (right): http://codereview.appspot.com/63188/diff/1/18#newcode95 Line 95: return; a warning would be nice here to say that the operation not completed... http://codereview.appspot.com/63188/diff/1/18#newcode103 Line 103: return; same as above http://codereview.appspot.com/63188/diff/1/18#newcode190 Line 190: IeBeaconTiming::TimestampToU16 (Time x) why not use a more significant variable name instead of x? http://codereview.appspot.com/63188/diff/1/19 File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.h (right): http://codereview.appspot.com/63188/diff/1/19#newcode49 Line 49: uint8_t m_aid; member attributes of a class should not be public and should not be accessed directly, they should be private and accessed only by accessor functions Getter and Setter above. http://codereview.appspot.com/63188/diff/1/20 File src/devices/mesh/dot11s/ie-dot11s-configuration.cc (right): http://codereview.appspot.com/63188/diff/1/20#newcode158 Line 158: //TODO: print to be done when? http://codereview.appspot.com/63188/diff/1/21 File src/devices/mesh/dot11s/ie-dot11s-configuration.h (right): http://codereview.appspot.com/63188/diff/1/21#newcode84 Line 84: bool acceptPeerLinks; please use the convention m_name and please make the member variables not public and use instead getters and setters. http://codereview.appspot.com/63188/diff/1/22 File src/devices/mesh/dot11s/ie-dot11s-id.cc (right): http://codereview.appspot.com/63188/diff/1/22#newcode111 Line 111: //TODO can you do it please? http://codereview.appspot.com/63188/diff/1/23 File src/devices/mesh/dot11s/ie-dot11s-id.h (right): http://codereview.appspot.com/63188/diff/1/23#newcode39 Line 39: //IeMeshId (char const meshId[32]); remove commented code http://codereview.appspot.com/63188/diff/1/28 File src/devices/mesh/dot11s/ie-dot11s-peering-protocol.cc (right): http://codereview.appspot.com/63188/diff/1/28#newcode63 Line 63: //TODO: print can you please print...? http://codereview.appspot.com/63188/diff/1/30 File src/devices/mesh/dot11s/ie-dot11s-perr.cc (right): http://codereview.appspot.com/63188/diff/1/30#newcode43 Line 43: // FILL please fill it... http://codereview.appspot.com/63188/diff/1/32 File src/devices/mesh/dot11s/ie-dot11s-prep.cc (right): http://codereview.appspot.com/63188/diff/1/32#newcode211 Line 211: //TODO can you do it please... http://codereview.appspot.com/63188/diff/1/34 File src/devices/mesh/dot11s/ie-dot11s-preq.cc (right): http://codereview.appspot.com/63188/diff/1/34#newcode133 Line 133: //IePreq::SetFlags (uint8_t flags) remove http://codereview.appspot.com/63188/diff/1/38 File src/devices/mesh/dot11s/peer-link-frame.cc (right): http://codereview.appspot.com/63188/diff/1/38#newcode131 Line 131: i.WriteHtolsbU16(m_capability); space before parentheses http://codereview.appspot.com/63188/diff/1/40 File src/devices/mesh/dot11s/peer-link.cc (right): http://codereview.appspot.com/63188/diff/1/40#newcode283 Line 283: PeerLink::StateMachine (PeerEvent event,PmpReasonCode reasoncode) this function is too long and it should be divided into several functions for a matter of maintability and readability. I would suggest in each case :IDLE,OPN_SNT,... to have a function that will treat the case. http://codereview.appspot.com/63188/diff/1/40#newcode293 Line 293: // TODO Callback MLME-SignalPeerLinkStatus when? http://codereview.appspot.com/63188/diff/1/40#newcode310 Line 310: {} should you put some code in here? is it an error to have values different from those above? http://codereview.appspot.com/63188/diff/1/40#newcode413 Line 413: // TODO Callback MLME-SignalPeerLinkStatus Please check all the TODOs in your code, thanks http://codereview.appspot.com/63188/diff/1/41 File src/devices/mesh/dot11s/peer-link.h (right): http://codereview.appspot.com/63188/diff/1/41#newcode224 Line 224: /// When last beacon was sent (TODO or received?) when you plan to do it? http://codereview.appspot.com/63188/diff/1/41#newcode255 Line 255: /// ? any comments? http://codereview.appspot.com/63188/diff/1/44 File src/devices/mesh/dot11s/peer-management-protocol.cc (right): http://codereview.appspot.com/63188/diff/1/44#newcode155 Line 155: if(i == m_neighbourBeacons.end()) coding style: space after if and two spaces after the return if (...) { } please change this for the whole code... http://codereview.appspot.com/63188/diff/1/44#newcode229 Line 229: bool reject = ! (ShouldAcceptOpen (interface, peerAddress,reasonCode)); why not having an accept boolean to avoid multiple negations: it would be after if (accept) instead of if (!reject)... http://codereview.appspot.com/63188/diff/1/44#newcode288 Line 288: NS_ASSERT (false); please use NS_ERROR instead and write an understanble error message http://codereview.appspot.com/63188/diff/1/44#newcode345 Line 345: if(peerLink != 0) remove 2 spaces indentation, again please verify indentation for the whole file. http://codereview.appspot.com/63188/diff/1/49 File src/devices/mesh/mesh-l2-routing-protocol.h (right): http://codereview.appspot.com/63188/diff/1/49#newcode84 Line 84: * Note that route discobery works async. -- RequestRoute returns immediately, while discovery http://codereview.appspot.com/63188/diff/1/49#newcode95 Line 95: * to really send packetusing routing information. space: packet using http://codereview.appspot.com/63188/diff/1/50 File src/devices/mesh/mesh-point-device.cc (right): http://codereview.appspot.com/63188/diff/1/50#newcode305 Line 305: NS_ASSERT(false); please generate an error, the return 0 statement will never be reached, so it is useless. http://codereview.appspot.com/63188/diff/1/50#newcode365 Line 365: return; why type in return? http://codereview.appspot.com/63188/diff/1/53 File src/devices/mesh/mesh-wifi-beacon.h (right): http://codereview.appspot.com/63188/diff/1/53#newcode56 Line 56: /// Create wifi header for beacon frame. \param address is sender address \param mpAddress is mesh point address please put parmeters in new lines http://codereview.appspot.com/63188/diff/1/55 File src/devices/mesh/mesh-wifi-interface-mac.cc (right): http://codereview.appspot.com/63188/diff/1/55#newcode191 Line 191: NS_LOG_DEBUG("SetWifiPhy: Can switch channel now: " << CanSwitchChannel() ); // TMP why is it TMP? will you need to remove it? http://codereview.appspot.com/63188/diff/1/55#newcode575 Line 575: if (beacon_hdr.GetSsid ().IsEqual(GetSsid())) space after IsEqual
Once again I'd like to thank Faker for thorough review. Some points are not answered yet, coming soon at https://forge.wenos.ru/hgprojects/ns3dev Now I propose to pause review until 3.5 release. http://codereview.appspot.com/63188/diff/1/2 File examples/mesh.cc (right): http://codereview.appspot.com/63188/diff/1/2#newcode70 Line 70: /// Create nodes and setup theis mobility On 2009/06/10 16:19:58, faker.moatamri wrote: > spelling: their Done. http://codereview.appspot.com/63188/diff/1/3 File src/devices/mesh/dot11s/airtime-metric.cc (right): http://codereview.appspot.com/63188/diff/1/3#newcode60 Line 60: AirtimeLinkMetricCalculator::CalculateMetric(Mac48Address peerAddress, Ptr<MeshWifiInterfaceMac> mac) On 2009/06/10 16:19:58, faker.moatamri wrote: > What kind of metric are you calculating here? some comments will be welcome like > those in the header file. Done. http://codereview.appspot.com/63188/diff/1/3#newcode70 Line 70: uint32_t metric = (uint32_t) ( On 2009/06/10 16:19:58, faker.moatamri wrote: > What is the 10240 number? or 8* 1e9? is it possible to use a constant in the > standard form instead of using numbers? Done. http://codereview.appspot.com/63188/diff/1/5 File src/devices/mesh/dot11s/dot11s-helper.cc (right): http://codereview.appspot.com/63188/diff/1/5#newcode73 Line 73: if (channel > 0) On 2009/06/10 16:19:58, faker.moatamri wrote: > Kill commented code... helpers reworked to device helper + mesh stack installer, see updated codebase. Commented code killed. http://codereview.appspot.com/63188/diff/1/5#newcode132 Line 132: //if(*root_iterator == i.GetDistanceFrom(c.Begin ())) On 2009/06/10 16:19:58, faker.moatamri wrote: > kill commented code Done. http://codereview.appspot.com/63188/diff/1/5#newcode144 Line 144: return Install (phy, NodeContainer (node), roots, nInterfaces); On 2009/06/10 16:19:58, faker.moatamri wrote: > Here you are iterating on a list of one node. Is it possible to implement the > Install for one node and then to call it from the iterator in the above > function: > for (NodeContainer::Iterator i = c.Begin (); i != c.End (); ++i) > { > Ptr<Node> node = *i; > Install(...,node,...) > } > I think it might make more sense to do it this way... It makes no difference http://codereview.appspot.com/63188/diff/1/6 File src/devices/mesh/dot11s/dot11s-helper.h (right): http://codereview.appspot.com/63188/diff/1/6#newcode57 Line 57: * \brief Install 802.11s mesh device & protocols on given node On 2009/06/10 16:19:58, faker.moatamri wrote: > This installs on given node list right? right. fixed. http://codereview.appspot.com/63188/diff/1/7 File src/devices/mesh/dot11s/dot11s-mac-header.cc (right): http://codereview.appspot.com/63188/diff/1/7#newcode113 Line 113: return; On 2009/06/10 16:19:58, faker.moatamri wrote: > May be should give here a warning or throw an error that we cannot have more > than 3 addresses? changed to NS_ASSERT (num_of_addresses <= 3); http://codereview.appspot.com/63188/diff/1/7#newcode256 Line 256: NS_ASSERT (false); On 2009/06/10 16:19:58, faker.moatamri wrote: > This will generate an assertion fault, the return above is useless since it will > stop anyway. I suggest putting output error since I guess this is an error (is > it?), it would make more sense to generate an error here, but if you really want > to keep NS_ASSERT, please output some text describing the error. All NS_ASSERT (false) are changed to NS_FATAL_ERROR (meaningful message) http://codereview.appspot.com/63188/diff/1/7#newcode275 Line 275: NS_ASSERT (false); On 2009/06/10 16:19:58, faker.moatamri wrote: > Same comment Done. http://codereview.appspot.com/63188/diff/1/7#newcode279 Line 279: // ??? On 2009/06/10 16:19:58, faker.moatamri wrote: > If this code needs to be put, please put a TODO to be clear that is missing > code. If the behavior is not specified yet, please put a comment on that. Done. http://codereview.appspot.com/63188/diff/1/7#newcode286 Line 286: return retval; On 2009/06/10 16:19:58, faker.moatamri wrote: > same comment Done. http://codereview.appspot.com/63188/diff/1/7#newcode330 Line 330: { On 2009/06/10 16:19:58, faker.moatamri wrote: > did you mean Dot11sMacHeaderBits? Nope, I mean Built-In-Self-Test. http://codereview.appspot.com/63188/diff/1/7#newcode340 Line 340: bool result (true); On 2009/06/10 16:19:58, faker.moatamri wrote: > Testing the serialization would be very recommended here to avoid unexpected > behavior. Nothing but serialization test follows. http://codereview.appspot.com/63188/diff/1/8 File src/devices/mesh/dot11s/dot11s-mac-header.h (right): http://codereview.appspot.com/63188/diff/1/8#newcode46 Line 46: void SetAddr4 (Mac48Address address); On 2009/06/10 16:19:58, faker.moatamri wrote: > What are those addresses? why 4,5,6... not 1,2,3...? This is known as 4- and 6-address extensions (or address schemes) for multihop wireless networks. Addresses 1 to 3 are already present in WifiMacHeader (frankly speaking address 4 is also there, but it is never used for management frames, so 802.11s has to redefine it in own header). Please let me know if more detailed answer is needed. http://codereview.appspot.com/63188/diff/1/10 File src/devices/mesh/dot11s/hwmp-mac-plugin.cc (right): http://codereview.appspot.com/63188/diff/1/10#newcode52 Line 52: { On 2009/06/10 16:19:58, faker.moatamri wrote: > In this function, I would recommand creating two private functions that will > treat the mesh header and the action respectively. This will increase > readability specially if you plan to add more functionalities in here. Done. http://codereview.appspot.com/63188/diff/1/10#newcode75 Line 75: NS_ASSERT(false); On 2009/06/10 16:19:58, faker.moatamri wrote: > Message saying that extension is not supported yet would be appreciated. Same > for all the NS_ASSERT(false) in your code. Done. http://codereview.appspot.com/63188/diff/1/10#newcode81 Line 81: return false; On 2009/06/10 16:19:58, faker.moatamri wrote: > your return will never be called right? Done. http://codereview.appspot.com/63188/diff/1/10#newcode196 Line 196: NS_ASSERT (!m_preqTimer.IsRunning()); On 2009/06/10 16:19:58, faker.moatamri wrote: > This line and the if (m_preqTimer.IsRunning ()) > return; > are doing the same job right? Right. I think extra assertion is harmless but can be useful. http://codereview.appspot.com/63188/diff/1/11 File src/devices/mesh/dot11s/hwmp-mac-plugin.h (right): http://codereview.appspot.com/63188/diff/1/11#newcode67 Line 67: //add a destination to exisying PREQ generated by me and stored in On 2009/06/10 16:19:58, faker.moatamri wrote: > existing, may be use the /** > * > */ Done. http://codereview.appspot.com/63188/diff/1/12 File src/devices/mesh/dot11s/hwmp-protocol.cc (right): http://codereview.appspot.com/63188/diff/1/12#newcode193 Line 193: HwmpProtocol::RequestRoute ( On 2009/06/10 16:19:58, faker.moatamri wrote: > This functionality might be given to another class to make the protocol class > less heavy, and it would be called by the protocol as a service. Will do this during the next refactoring wave. http://codereview.appspot.com/63188/diff/1/12#newcode208 Line 208: NS_ASSERT (false); On 2009/06/10 16:19:58, faker.moatamri wrote: > Again the NS_ASSERT(false)... please replace all of them in your code by > NS_ERROR and please write a user friendly message to explain the error or the > not yet supported feature. Done. http://codereview.appspot.com/63188/diff/1/12#newcode220 Line 220: return false; On 2009/06/10 16:19:58, faker.moatamri wrote: > again return never reached... Done. http://codereview.appspot.com/63188/diff/1/12#newcode239 Line 239: if(*chan == plugin->second->GetChannelId ()) On 2009/06/10 16:19:58, faker.moatamri wrote: > for a matter of clarity, you should may be use brackets here to make it more > readable. Done. http://codereview.appspot.com/63188/diff/1/12#newcode472 Line 472: //chack if must retransmit: On 2009/06/10 16:19:58, faker.moatamri wrote: > check Done. http://codereview.appspot.com/63188/diff/1/12#newcode733 Line 733: retval.pkt = NULL; On 2009/06/10 16:19:58, faker.moatamri wrote: > Please use 0 instead, please refer to coding style. Done. http://codereview.appspot.com/63188/diff/1/12#newcode764 Line 764: while (1) On 2009/06/10 16:19:58, faker.moatamri wrote: > why not put while (packet.pkt != 0) instead of the infinite loop? Done. http://codereview.appspot.com/63188/diff/1/12#newcode767 Line 767: if (packet.pkt == NULL) On 2009/06/10 16:19:58, faker.moatamri wrote: > 0 instead of NULL Done. http://codereview.appspot.com/63188/diff/1/12#newcode833 Line 833: while (1) On 2009/06/10 16:19:58, faker.moatamri wrote: > same as above Done. http://codereview.appspot.com/63188/diff/1/13 File src/devices/mesh/dot11s/hwmp-protocol.h (right): http://codereview.appspot.com/63188/diff/1/13#newcode101 Line 101: ///\brief forms a path error information element when list of destination fails on a given interface On 2009/06/10 16:19:58, faker.moatamri wrote: > This class is too huge, it should delegate some responsibilities to other > classes, it will be more readable and more maintainable and eventually reusable. > For example Path error handling can be in a separate class. Same thing for > queuing and dequeuing,stats... Always try to make the classes sepcialized and > their functionalities limited to ease, readibility, maintainability, > extensibility and reusability. Agree. Will refactor HwmpProtocol in the next refactoring cycle. http://codereview.appspot.com/63188/diff/1/14 File src/devices/mesh/dot11s/hwmp-rtable.cc (right): http://codereview.appspot.com/63188/diff/1/14#newcode230 Line 230: //We suppose that no dublicates here can be On 2009/06/10 16:19:58, faker.moatamri wrote: > duplicates Done. http://codereview.appspot.com/63188/diff/1/14#newcode311 Line 311: void HwmpRtableTest::Test1 () On 2009/06/10 16:19:58, faker.moatamri wrote: > Please give a more signicative name to the tests you are executing, in order to > give an idea of what you are doing or add some comments about it. Will do this soon. http://codereview.appspot.com/63188/diff/1/14#newcode347 Line 347: // TODO: test AddPrecursor and GetPrecursors On 2009/06/10 16:19:58, faker.moatamri wrote: > will you be able to do it soon? Yes, will do this soon. http://codereview.appspot.com/63188/diff/1/18 File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.cc (right): http://codereview.appspot.com/63188/diff/1/18#newcode95 Line 95: return; On 2009/06/10 16:19:58, faker.moatamri wrote: > a warning would be nice here to say that the operation not completed... Done. http://codereview.appspot.com/63188/diff/1/18#newcode103 Line 103: return; On 2009/06/10 16:19:58, faker.moatamri wrote: > same as above Done. http://codereview.appspot.com/63188/diff/1/18#newcode190 Line 190: IeBeaconTiming::TimestampToU16 (Time x) On 2009/06/10 16:19:58, faker.moatamri wrote: > why not use a more significant variable name instead of x? Renamed to t :)) http://codereview.appspot.com/63188/diff/1/19 File src/devices/mesh/dot11s/ie-dot11s-beacon-timing.h (right): http://codereview.appspot.com/63188/diff/1/19#newcode49 Line 49: uint8_t m_aid; On 2009/06/10 16:19:58, faker.moatamri wrote: > member attributes of a class should not be public and should not be accessed > directly, they should be private and accessed only by accessor functions Getter > and Setter above. Done. http://codereview.appspot.com/63188/diff/1/20 File src/devices/mesh/dot11s/ie-dot11s-configuration.cc (right): http://codereview.appspot.com/63188/diff/1/20#newcode158 Line 158: //TODO: print On 2009/06/10 16:19:58, faker.moatamri wrote: > to be done when? TODO removed http://codereview.appspot.com/63188/diff/1/21 File src/devices/mesh/dot11s/ie-dot11s-configuration.h (right): http://codereview.appspot.com/63188/diff/1/21#newcode84 Line 84: bool acceptPeerLinks; On 2009/06/10 16:19:58, faker.moatamri wrote: > please use the convention m_name and please make the member variables not public > and use instead getters and setters. IMO in this case this is an overkill. http://codereview.appspot.com/63188/diff/1/22 File src/devices/mesh/dot11s/ie-dot11s-id.cc (right): http://codereview.appspot.com/63188/diff/1/22#newcode111 Line 111: //TODO On 2009/06/10 16:19:58, faker.moatamri wrote: > can you do it please? Done. http://codereview.appspot.com/63188/diff/1/23 File src/devices/mesh/dot11s/ie-dot11s-id.h (right): http://codereview.appspot.com/63188/diff/1/23#newcode39 Line 39: //IeMeshId (char const meshId[32]); On 2009/06/10 16:19:58, faker.moatamri wrote: > remove commented code Done. http://codereview.appspot.com/63188/diff/1/28 File src/devices/mesh/dot11s/ie-dot11s-peering-protocol.cc (right): http://codereview.appspot.com/63188/diff/1/28#newcode63 Line 63: //TODO: print On 2009/06/10 16:19:58, faker.moatamri wrote: > can you please print...? Done. http://codereview.appspot.com/63188/diff/1/30 File src/devices/mesh/dot11s/ie-dot11s-perr.cc (right): http://codereview.appspot.com/63188/diff/1/30#newcode43 Line 43: // FILL On 2009/06/10 16:19:58, faker.moatamri wrote: > please fill it... will be done asap http://codereview.appspot.com/63188/diff/1/32 File src/devices/mesh/dot11s/ie-dot11s-prep.cc (right): http://codereview.appspot.com/63188/diff/1/32#newcode211 Line 211: //TODO On 2009/06/10 16:19:58, faker.moatamri wrote: > can you do it please... will be done asap http://codereview.appspot.com/63188/diff/1/34 File src/devices/mesh/dot11s/ie-dot11s-preq.cc (right): http://codereview.appspot.com/63188/diff/1/34#newcode133 Line 133: //IePreq::SetFlags (uint8_t flags) On 2009/06/10 16:19:58, faker.moatamri wrote: > remove Done. http://codereview.appspot.com/63188/diff/1/38 File src/devices/mesh/dot11s/peer-link-frame.cc (right): http://codereview.appspot.com/63188/diff/1/38#newcode131 Line 131: i.WriteHtolsbU16(m_capability); On 2009/06/10 16:19:58, faker.moatamri wrote: > space before parentheses Done. http://codereview.appspot.com/63188/diff/1/40 File src/devices/mesh/dot11s/peer-link.cc (right): http://codereview.appspot.com/63188/diff/1/40#newcode283 Line 283: PeerLink::StateMachine (PeerEvent event,PmpReasonCode reasoncode) On 2009/06/10 16:19:58, faker.moatamri wrote: > this function is too long and it should be divided into several functions for a > matter of maintability and readability. I would suggest in each case > :IDLE,OPN_SNT,... to have a function that will treat the case. I'm afraid of touching state machine. The next one will be more structured, I promise :) http://codereview.appspot.com/63188/diff/1/40#newcode293 Line 293: // TODO Callback MLME-SignalPeerLinkStatus On 2009/06/10 16:19:58, faker.moatamri wrote: > when? soon http://codereview.appspot.com/63188/diff/1/40#newcode310 Line 310: {} On 2009/06/10 16:19:58, faker.moatamri wrote: > should you put some code in here? is it an error to have values different from > those above? NS_FATAL_ERROR ("") inserted http://codereview.appspot.com/63188/diff/1/40#newcode413 Line 413: // TODO Callback MLME-SignalPeerLinkStatus On 2009/06/10 16:19:58, faker.moatamri wrote: > Please check all the TODOs in your code, thanks Done. http://codereview.appspot.com/63188/diff/1/41 File src/devices/mesh/dot11s/peer-link.h (right): http://codereview.appspot.com/63188/diff/1/41#newcode224 Line 224: /// When last beacon was sent (TODO or received?) On 2009/06/10 16:19:58, faker.moatamri wrote: > when you plan to do it? Done. http://codereview.appspot.com/63188/diff/1/41#newcode255 Line 255: /// ? On 2009/06/10 16:19:58, faker.moatamri wrote: > any comments? Done. http://codereview.appspot.com/63188/diff/1/44 File src/devices/mesh/dot11s/peer-management-protocol.cc (right): http://codereview.appspot.com/63188/diff/1/44#newcode229 Line 229: bool reject = ! (ShouldAcceptOpen (interface, peerAddress,reasonCode)); On 2009/06/10 16:19:58, faker.moatamri wrote: > why not having an accept boolean to avoid multiple negations: > it would be after if (accept) instead of if (!reject)... I see no difference http://codereview.appspot.com/63188/diff/1/44#newcode288 Line 288: NS_ASSERT (false); On 2009/06/10 16:19:58, faker.moatamri wrote: > please use NS_ERROR instead and write an understanble error message Done. http://codereview.appspot.com/63188/diff/1/44#newcode345 Line 345: if(peerLink != 0) On 2009/06/10 16:19:58, faker.moatamri wrote: > remove 2 spaces indentation, again please verify indentation for the whole file. Done. http://codereview.appspot.com/63188/diff/1/49 File src/devices/mesh/mesh-l2-routing-protocol.h (right): http://codereview.appspot.com/63188/diff/1/49#newcode84 Line 84: * Note that route discobery works async. -- RequestRoute returns immediately, while On 2009/06/10 16:19:58, faker.moatamri wrote: > discovery Done. http://codereview.appspot.com/63188/diff/1/49#newcode95 Line 95: * to really send packetusing routing information. On 2009/06/10 16:19:58, faker.moatamri wrote: > space: packet using Done. http://codereview.appspot.com/63188/diff/1/50 File src/devices/mesh/mesh-point-device.cc (right): http://codereview.appspot.com/63188/diff/1/50#newcode305 Line 305: NS_ASSERT(false); On 2009/06/10 16:19:58, faker.moatamri wrote: > please generate an error, the return 0 statement will never be reached, so it is > useless. Done. http://codereview.appspot.com/63188/diff/1/50#newcode365 Line 365: return; On 2009/06/10 16:19:58, faker.moatamri wrote: > why type in return? I guess it wasn't void some time ago. Fixed. http://codereview.appspot.com/63188/diff/1/53 File src/devices/mesh/mesh-wifi-beacon.h (right): http://codereview.appspot.com/63188/diff/1/53#newcode56 Line 56: /// Create wifi header for beacon frame. \param address is sender address \param mpAddress is mesh point address On 2009/06/10 16:19:58, faker.moatamri wrote: > please put parmeters in new lines Done. http://codereview.appspot.com/63188/diff/1/55 File src/devices/mesh/mesh-wifi-interface-mac.cc (right): http://codereview.appspot.com/63188/diff/1/55#newcode191 Line 191: NS_LOG_DEBUG("SetWifiPhy: Can switch channel now: " << CanSwitchChannel() ); // TMP On 2009/06/10 16:19:58, faker.moatamri wrote: > why is it TMP? will you need to remove it? Done. http://codereview.appspot.com/63188/diff/1/55#newcode575 Line 575: if (beacon_hdr.GetSsid ().IsEqual(GetSsid())) On 2009/06/10 16:19:58, faker.moatamri wrote: > space after IsEqual Done.
