Add LEACH routing protocol

[n-jay]

Adds the LEACH protocol to existing routing protocol selection.

Addresses issue #928

[n-jay]

Fingerprint values seem to be changing with each subsequent Github Actions run.

[rhornig]

1st. Thanks a lot for the PR. We are busy with other topics right now, but we will review this for the next INET release. Until then:

As other fingerprints are stable, this indicates that the code execution is not repeatable. In my experience this might be caused by:

• some uninitalized variables lurking in the code (not very likely as it would rather crash the process)
• using undefined behavior in C++ code (not likely: hopefully you don't do any nasty things in your code :)
• iterating over an unordered collection (this is the most likely cause)

It is very easy to create a hashed map or set and forget about the fact that its iteration order may depend on memory layout. We had some similar issues in early versions of INET and OMNeT++.

[rhornig]

I just had a quick look into the code (not a proper review, but some comments).

• license is fine (LGPL), but please use similar format for file headers like the rest of the code (i.e. use just SPDX-License-Identifier: LGPL-3.0-or-later) This reduces noise and still does the job.
• including headers files must not assume that the current directory is on the include path (i.e. prefix with inet/routing/leach even for your local headers). This is done because some other projects using INET may have a local header file similarly named (for whatever reason) and hell will broke...
• I would put the whole codebase under the inet::leach namespace (instead of the top level ::inet)
• use proper enums. packetType == 5 does not mean anything to the code reader. It's an implementation detail that the value is currently 5.
• the check for a single wireless interface is very fragile. You should not assume that a wireless interface is called "wlan". It may change in the future, or you may want to use leach on a mobile connection (i.e. from Simu5G). I would rather have a parameter on the protocol module that specifies the interface that must be used. Its default value can be "wlan0", so you don't have to count the interfaces or know the name in advance). If need arise the user still can change the default value.
• finally regarding the changing fingerprints. I see an include <unordered_set> in LeachBS.cc. Check that file carefully whether the does not depend of that set's order.

[n-jay]

Noted @rhornig, thank you!
I'm still very much a novice when it comes to C++ so this was extremely helpful. I'll look into the points you made and do the necessary changes.

Can those changes be committed separately beyond this point (with detailed commit messages), or should I squash them to the earlier commits?

[rhornig]

just go on with new commits. Once we decide to merge, we will squash it.

[n-jay]

I see an include <unordered_set> in LeachBS.cc. Check that file carefully whether the does not depend of that set's order.

@rhornig I looked into the use of unordered collections in the LeachBS.cc file and it turned out to be an unused #includes from my original code that had no implementation. I've since removed it in 298eff7.
I've gone over the rest of my .cc and .h files and verified that there aren't any more unordered collections, only vectors. However the problem still seems to persist.

[rhornig]

Regarding the failing fingerprint tests: If there is no obviously visible parts where unordered collections are used then I would check also for memory issues. Running valgrind memcheck may provide some tips. I would also check whether release and debug versions produce the same fingerprint. If they do, I would try to somehow reproduce the different fingerprints reliably locally (having a different fingerprint only during the github ci test makes catching this bug very hard).

Once you can reproduce the fingerprints, you can write out an event log file for both runs and compare those and see where the first difference happens. That would at least give some hints what is happening.

These are the ugliest type of errors one can find in a simulation. That's the main reason we have implemented fingerprinting, to at least catch them early.

[rhornig]

We will need some documentation about the routing protocol implementation in the Leach.ned file.:

• What protocol it implements (possibly an URL to the standard)
• what features are / are not implemented compared to that standard document. WHat is the difference compared to the standard.
• implementation specific documentation, like what parameters must be configured by the user and what are th elimitation (e.g. does it work only on wireless interfaces? Is there limitations on what kind of wireless protocol can be used? Wifi, 5G etc. Are there limitation on the number of interfaces (as far as I see only a single wireless interfaces is supported).

[rhornig]

For the interface name parameter: We have similar code patterns (i.e. for selecting the name of the interface to be used) in Dymo and GPSR. The parameter is called interfaces and you can use a cPatternMatcher to iterate and match the selected interface (see the related .cc files). This approach gives the advantage that you can use pattern characters in the name of the interface and potentially also allows to support binding to more than one interface. If your implementation support only a single interface, I would still name it interfaces and would comment there that only a single interface is allowed (as it is an implementation restriction). You can still throw errors if more than one interface name matches.

https://github.com/inet-framework/inet/blob/master/src/inet/routing/dymo/Dymo.cc#L1294

[n-jay]

Regarding the failing fingerprint tests: If there is no obviously visible parts where unordered collections are used then I would check also for memory issues. Running valgrind memcheck may provide some tips. I would also check whether release and debug versions produce the same fingerprint. If they do, I would try to somehow reproduce the different fingerprints reliably locally (having a different fingerprint only during the github ci test makes catching this bug very hard).

Will look into this @rhornig
I also have a hunch on what might be causing this. So will check it out, run the simulation and get back to you.

We will need some documentation about the routing protocol implementation in the Leach.ned file.:

Noted, will add that.
Would documentation that looks like the Dymo.ned and Gpsr.ned be sufficient?

For the interface name parameter: We have similar code patterns (i.e. for selecting the name of the interface to be used) in Dymo and GPSR.

Noted, will study them and see how the code can be modified.
My initial code was based on the Dsdv implementation which I think is a bit dated.

inet/src/inet/routing/dsdv/Dsdv.cc

Lines 72 to 83 in 3a44054

	int num_80211 = 0;
	NetworkInterface *ie;
	NetworkInterface *i_face;
	broadcastDelay = &par("broadcastDelay");
	for (int i = 0; i < ift->getNumInterfaces(); i++) {
	ie = ift->getInterface(i);
	if (ie->isWireless()) {
	i_face = ie;
	num_80211++;
	interfaceId = i;
	}
	}

Once this is merged, I can work on updating that to match Dymo and Gpsr if its ok.

[rhornig]

Documentation: keep it short. Refer to the original specification and mention only differences / limitations compared to that and parameters and behavior specific to this current implementation (i.e. mention possible statistics gathered, limitation (like only a single wireless interface supported etc). Think about a NEW user who is otherwise already familiar with the leach protocol (by reading the spec).

[n-jay]

i.e. it must work with any UDP application already available in INET)

@rhornig, I'm currently working on this and need a bit of clarification. Is it required that all packet types (control, acknowledgment, etc.) used in LEACH be UDP packets or just the packets with sensed data?

I feel like the former (all packets UDP) is the way to go because otherwise, it's going to get confusing when filtering packets in the processMessage method.
I can drop the setPacketType use and instead use the setKind to perform the filtering.
Let me know if this is the ideal approach.

[n-jay]

@rhornig, I implemented UDP packets in 6008a9a.
Appreciate some feedback on whether this approach is correct so I can replicate it for all packet transmissions

inet/src/inet/routing/leach/Leach.cc

Line 282 in 6008a9a

void Leach::broadcastCtrlToNCH() {

inet/src/inet/routing/leach/Leach.cc

Line 358 in 6008a9a

void Leach::sendDataToCH(Ipv4Address nodeAddr, Ipv4Address CHAddr,

[n-jay]

Hi @rhornig,
I started finalizing the UDP packet support, but an issue with my test simulation parameters is currently holding me back.
Instead of just writing it here, I've created a new question in the discussion section to benefit others: #976.

I would really appreciate some feedback on this so I can quickly finalize the rest of the protocol.

[n-jay]

Hi @rhornig

I started finalizing the UDP packet support, but an issue with my test simulation parameters is currently holding me back.
Instead of just writing it here, I've created a new question in the discussion section to benefit others: #976.

I made multiple attempts to solve the simulation issue based on the showcases, tutorials and examples in INET but had no luck.
I even checked out to my code to one of earliest commits (67c97be) assuming the issue must be resulting from one of the subsequent changes I've made based on the review comments, but it made no difference. This same codebase works fine in INET 4.2.5 (on OMNET v5).

I set debug points and identified that the CH election in the protocol works as expected, reaching the packet transmission point without any exceptions or issues:
https://github.com/n-jay/inet/blob/f4a4040557663d453a91fb43b81282c27cef8166/src/inet/routing/leach/Leach.cc#L307

But the issue is that the Non-CH nodes seem to be receiving the sent packets, and the debug points set within the processMessage method are not triggered.
https://github.com/n-jay/inet/blob/f4a4040557663d453a91fb43b81282c27cef8166/src/inet/routing/leach/Leach.cc#L130

Would greatly appreciate some feedback on this because this is a major blocker preventing me from finalizing the protocol.

[n-jay]

Would greatly appreciate some feedback on this because this is a major blocker preventing me from finalizing the protocol.

@rhornig, happy to inform that the issue is resolved with commit 162a92b!
Described the fix in the discussion comment: #976 (comment).
Will now proceed to finalize the rest of the protocol quickly and update you here.