I don't like the proposed patch because:

- The model is not necessarily limited to the speed of light. E.g., if you use it with the UAN module, you need the propagation speed of acoustic waves in water. This rules out both renaming the attribute to "SpeedOfLight" and the alternative proposal of using permittivity and permeability as the configurable attributes. 

- The setting should be a member attribute rather than a global one, so as to allow different model instances to coexist in a simulation and have different propagation speeds. Consider for instance a scenario with a mix of over-the-air transmission in the atmosphere plus satellite communications in the vacuum, maybe together with optical transmissions over fiber that have yet another propagation speed...


Based on the above considerations, I applied a minimal fix leaving the member attribute as is, and just changing the default value to the speed of light in the vacuum as proposed by Tommaso.

changeset:   11038:c182dac1058a
tag:         tip
user:        Nicola Baldo <nbaldo@cttc.es>
date:        Tue Oct 28 11:48:13 2014 +0100
summary:     fixed Bug 2011 - speed of light constant


This new change makes the tests routing-olsr-regression and devices-mesh-dot11s-regression fail. I guess it might be due to the propagation delay differences, if so we could just regenerate the pcap traces. However I was unable to check that, I tried tracediff (got no output) and wireshark (merging traces, but got visible difference)... do you have any suggestion?

for wireshark I meant that I got NO visible difference

(In reply to Nicola Baldo from comment #2)
> for wireshark I meant that I got NO visible difference


There is a small difference; for instance, in OLSR's 'bug780-0-0.pcap', the diff is:

@@ -586,9 +586,9 @@
 140.000000 IP 10.1.1.1 > 10.1.1.2: ICMP echo request, id 0, seq 90, length 64
 140.001467 Acknowledgment RA:00:00:00:00:00:06 
 140.002669 IP 10.1.1.1 > 10.1.1.2: ICMP echo request, id 0, seq 90, length 64
-140.004500 Acknowledgment RA:00:00:00:00:00:07 
-140.005962 IP 10.1.1.2 > 10.1.1.1: ICMP echo reply, id 0, seq 90, length 64
-140.005972 Acknowledgment RA:00:00:00:00:00:08 
+140.004501 Acknowledgment RA:00:00:00:00:00:07 
+140.005963 IP 10.1.1.2 > 10.1.1.1: ICMP echo reply, id 0, seq 90, length 64
+140.005973 Acknowledgment RA:00:00:00:00:00:08 
 140.029268 IP 10.1.1.1.698 > 10.1.1.255.698: OLSRv4, seq 0x0046, length 28
 140.111976 IP 10.1.1.3.698 > 10.1.1.255.698: OLSRv4, seq 0x0047, length 60
 141.000000 IP 10.1.1.1 > 10.1.1.2: ICMP echo request, id 0, seq 91, length 64

I will take care of this; the way that I typically handle pcap divergence is:

1) run the test suite with the '-t out.txt -r' options.  -r retains new pcaps in the testpy-output directory.  '-t out.txt' will tell you which pcap is failing.

2) check that the divergence is expected, by tcpdumping both old and new pcaps and running the resulting text files through diff

3) if the differences look sensible, rerun the test with the '-u' option, and it will regenerate new traces that you can check in; e.g.

./test.py -s routing-olsr-regression -u

Another solution is to reset the attribute default in the test to what it was prior to the patch (tests should pass) but that is not as nice to maintain over time.

The underlying issue is that it is a burden to have unrelated routing protocol tests fail on changes such as these, so I will see about changing the routing tests to avoid trace file comparison.

p.s. I am fine with your patch instead of mine

(In reply to Nicola Baldo from comment #1) [my paraphrase]:
> - The model is not necessarily limited to the speed of light. E.g., ... 
> propagation speed of acoustic waves in water. 
> 
> - The setting should be a member attribute rather than a global one, so as
> to allow different model instances to coexist in a simulation and have
> different propagation speeds. Consider ... a mix of
> over-the-air transmission plus communications in
> the vacuum, together with transmissions over fiber

I don't like the proposals because they still leave us open to inconsistent values for `c` (the speed of light in vacuum) between different models that intend to refer to `c`.

I suggest we have a global value for `c` (so modelers can choose to tweak globally, if that is convenient, for example to adjust everything to air), and add attributes for propagation speed (really should be group velocity) to any model that needs a value.

For over-the-air and vacuum channels the attribute initial default should reference the global value (hmm, can we do that?  dynamically pick up a GlobalValue as the initial value of an attribute?)  

For other media, reasonable defaults are

- wired (e.g, copper)  0.5 `c`  (average value of plenum rated Cat-5)
- fiber                0.62 `c` (1 / n for index of refraction n = 1.62)
- water                0.75 `c` (n = 1.33)

(In reply to Peter Barnes from comment #4)
> (In reply to Nicola Baldo from comment #1) [my paraphrase]:
> > - The model is not necessarily limited to the speed of light. E.g., ... 
> > propagation speed of acoustic waves in water. 
> > 
> > - The setting should be a member attribute rather than a global one, so as
> > to allow different model instances to coexist in a simulation and have
> > different propagation speeds. Consider ... a mix of
> > over-the-air transmission plus communications in
> > the vacuum, together with transmissions over fiber
> 
> I don't like the proposals because they still leave us open to inconsistent
> values for `c` (the speed of light in vacuum) between different models that
> intend to refer to `c`.

what other models are referring to `c`?  (i.e. what more are you trying to solve?)

Presently, a mix of technologies with different speeds should be able to be handled by setting the attribute differently each time a topology helper is invoked (`Install()` methods tend to run with the channel type).

I'm OK with what Nicola committed since it is the most minimal patch, assuming that the scope of what we are trying to improve is just `Speed` in that model (I'm not sure what are the other references to speed of light in the simulator).

> 
> I suggest we have a global value for `c` (so modelers can choose to tweak
> globally, if that is convenient, for example to adjust everything to air),
> and add attributes for propagation speed (really should be group velocity)
> to any model that needs a value.
> 
> For over-the-air and vacuum channels the attribute initial default should
> reference the global value (hmm, can we do that?  dynamically pick up a
> GlobalValue as the initial value of an attribute?)  

No, we want to avoid coupling between attribute default values and global values (static initialization order), which is why I suggested in my other patch to delete `Speed` if we were to promote this to a global value.

> 
> For other media, reasonable defaults are
> 
> - wired (e.g, copper)  0.5 `c`  (average value of plenum rated Cat-5)
> - fiber                0.62 `c` (1 / n for index of refraction n = 1.62)
> - water                0.75 `c` (n = 1.33)

I guess I would lean towards either keeping what we have now, or if we pull the value `c` out, I would tend towards making `c` a static global constant, deleting the `Speed` attribute, and replacing it with a `RefractiveIndex` attribute.  

If we have something explicitly labeled `c` we probably shouldn't make it a variable since it is a universally defined constant.

Two years with no further comment... I think we can consider the already committed patch as satisfactory for now. 

Feel free to reopen if you would like to propose a new patch. In that case, my preference would go for the static constant approach mentioned by Tom.