boulis / Castalia

An OMNeT-based simulator for low-power wireless networks such as Wireless Sensor Networks and Body Area Networks
80 stars 49 forks source link

isChannelClear() in Radio.cc may be missing call to Enter_Method() #11

Open jamesncl opened 6 years ago

jamesncl commented 6 years ago

The function isChannelClear() in Radio.cc is called directly by MAC modules. We have discussed this before in the Google group

However the function does not begin with a call to Omnet's Enter_Method() function. I've just read this in the Omnet++ manual, under section 4.12 Direct Method Calls Between Modules:

The second issue is how to let the simulation kernel know that a method call across modules is taking place. Why is this necessary in the first place? First, the simulation kernel always has to know which module’s code is currently executing, in order for ownership handling and other internal mechanisms to work correctly. Second, the Tkenv simulation GUI can animate method calls, but to be able to do that, it has to know about them. Third, method calls are also recorded in the event log. The solution is to add the Enter_Method() or Enter_Method_Silent() macro at the top of the methods that may be invoked from other modules. These calls perform context switching, and, in case of Enter_Method(), notify the simulation GUI so that animation of the method call can take place. Enter_Method_Silent() does not animate the method call, but otherwise it is equivalent Enter_Method().

Therefore I think we need a call to Enter_Method or Enter_Method_Silent at the beginning of isChannelClear() in Radio.cc


CCA_result Radio::isChannelClear()
{
    Enter_Method_Silent();

or

CCA_result Radio::isChannelClear()
{
    Enter_Method("isChannelClear(void)");

Simulations appear to work fine without this function call, which is surprising. I came across this issue when working on my own modules - I received an Omnet runtime error when I had a submodule of a compound module (Cca) call another submodule (Controller):

<!> Error in module (BoxMacTwoCca) SN.node[0].Communication.MAC.Cca (id=24) at event #1880, t=0.029048485641: scheduleAt() of module (BoxMacTwoController)SN.node[0].Communication.MAC.Controller called in the context of module (BoxMacTwoCca)SN.node[0].Communication.MAC.Cca: method called from the latter module lacks Enter_Method() or Enter_Method_Silent()?.

I'm not sure why this error is not raised when Castalia MAC modules directly call the radio isChannelClear() fucntion. So perhaps there is no benefit to fixing this. However adding it doesn't appear to affect performance (based on some simple test runs), appears to be the correct thing to do, and it may perhaps avoid problems in the future.

PS In all other functions I can find which are called directly from other modules (ResourceManager and SensorManager), this is the only instance I can find which is missing a call to Enter_Method.

boulis commented 6 years ago

Thank you James once again, for your input. Catching bugs and in general scrutinizing the code is what makes it better.

I am not sure what to do about this one. My inclination is to leave it as is. I understand the arguments for using Enter_Method() and I think they have a weak case in Castalia. We don't care about animating calls, or record them in a log, and having the kernel know which module is executing seems to have a rather vague value. One argument is that with more changes in Castalia, or modifications by other people (that might want to use the GUI animations for example) Enter_Method might become relevant. It's the "correct way" to do it. As a counterargument, we would have to do some testing to make sure nothing breaks. The manual refers to OMNeT 5.x and Castalia relies on 4.x.

The methods that are already use Enter_Method are either not currently used in Castalia (e.g., RAM related methods) or they should not use it (e.g., consumeEnergy, because it is not called by other modules). Actually there is one method that is used and is called by other modules: getSpentEnergy. So maybe it's safe to add it in isChannelClear as well.

I am not sure why you got an error in your case, these omissions should not cause an error (at least the way they are used in Castalia)

Happy to hear more arguments for or against.

jamesncl commented 6 years ago

After a little digging, the error I got when calling a module member function directly from another module was caused by this bit of code in Omnet's cSimpleModule:

int cSimpleModule::sendDelayed(cMessage *msg, simtime_t delay, cGate *outgate)
{
...
    if (this!=simulation.getContextModule() && simulation.getContextModule()!=NULL)
            throw cRuntimeError("send()/sendDelayed() of module (%s)%s called in the context of "
                                "module (%s)%s: method called from the latter module "
                                "lacks Enter_Method() or Enter_Method_Silent()? "

There is a check in sendDirect, scheduleAt and sendDelayed functions when a module sends a message, which checks that the execution "context module" is the sending module.

So, I think the only circumstances Enter_Method becomes important for Castalia (given as you say, there is no GUI) is when a module directly calls another module function, and that function also sends a message. This was the case for my MAC module. It has 2 submodules called CCA and Controller. CCA called Controller directly, and in the Controller function a timer was set and a message was sent, but in the execution context of CCA. Hence I got an error. In the case of Castalia calling isChannelClear() in Radio.cc, no messages or timers are sent, so there is no error.

I agree that changing a fundamental function which will be used by all users of Castalia is always risky without some thorough testing. I can only say that it hasn't broken anything on my current simulations. Therefore I would be happy to leave it as is. Perhaps a comment in the code would be useful, to indicate that the function isChannelClear is called directly by MAC modules, and that Enter_Method() may be required if that function ever changes to include a timer or message send.

James

boulis commented 6 years ago

Thank you for digging and finding where your error came from . That's definitely a situation that deserves at least a warning. As you note, we are not in the situation with isChannelClear().

A comment in the code might be a good idea. I'll have to see how to phrase it.