[SATELLITE LINKS] PR 2: FreeSpace Channels#556
Conversation
zigen
left a comment
There was a problem hiding this comment.
Reviewed 6 of 13 files at r1, all commit messages.
Reviewable status: 6 of 13 files reviewed, 7 unresolved discussions (waiting on @pfittipaldi)
quisp/channels/channels.ned line 15 at r1 (raw file):
} channel FSChannel extends DatarateChannel {
FreeSpaceChannel is better name. because FS seems hard to understand.
quisp/channels/channels.ned line 19 at r1 (raw file):
double cost = default(1); double distance @unit(km) = default(20km) @mutable; string distance_CSV;
keep variable name consistent. in this case, it should be distance_csv.
don't mix the capital case and lower case.
and also I'm not sure whether I should put csv content or csv file path.
quisp/channels/channels.ned line 20 at r1 (raw file):
double distance @unit(km) = default(20km) @mutable; string distance_CSV; double speed_of_light_in_FS @unit(m) = default(299792.458km);
keep the naming consitent. it should be speed_of_light_in_freespace
FS would confuse us by wrongly assuming it's femto-second
quisp/channels/channels.ned line 22 at r1 (raw file):
double speed_of_light_in_FS @unit(m) = default(299792.458km); double orbital_period @unit(s) = default(24h); bool CSV_varies_delay = default(false);
same, csv_varies_delay
quisp/channels/channels.ned line 56 at r1 (raw file):
double transmitter_telescope_diameter @unit(m) = default(13.5cm); double receiver_telescope_diameter @unit(m) = default(1m); double Fried_parameter @unit(m) = default(10cm);
fried_parameter
quisp/channels/FSChannel.h line 36 at r1 (raw file):
public: FSChannel(); void set_orbit_parameters(double orb_period, double orb_vis_start_coeff, double orb_vis_end_coeff);
functions and methods should be camelCase.
setOrbitParameters
quisp/channels/FSChannel.h line 39 at r1 (raw file):
bool checkLOS(); double getDistanceAtTime(const simtime_t time); SimTime getNext_check_time();
getNextCheckTime
zigen
left a comment
There was a problem hiding this comment.
Reviewed 3 of 13 files at r1.
Reviewable status: 9 of 13 files reviewed, 14 unresolved discussions (waiting on @pfittipaldi)
quisp/channels/FSChannel.h line 26 at r1 (raw file):
using namespace quisp::messages; struct ORBITAL_PARAMETERS {
struct name should be PascalCase, so OrbitalParameters. and explain the members. what is the vis?
quisp/channels/FSChannel.h line 43 at r1 (raw file):
private: ORBITAL_PARAMETERS op; CSVParser *dist_par;
why not dist_parser ? par sounds like some parameter.
quisp/channels/FSChannel.cc line 36 at r1 (raw file):
result = cDatarateChannel::processMessage(msg, options, t); } return result;
use the early-return pattern like this.
Suggestion:
if (!checkLOS() and !dynamic_cast<OspfPacket *>(msg)) {
Result result;
result.discard = true;
return result;
}
recalculateChannelParameters();
return cDatarateChannel::processMessage(msg, options, t);quisp/channels/FSChannel.cc line 39 at r1 (raw file):
} bool FSChannel::checkLOS() {
the function name checkLOS is not good, because we cannot guess what true and false means.
quisp/channels/FSChannel.cc line 45 at r1 (raw file):
return true; } return false;
consider make it simple
Suggestion:
return (currentTime >= op.vis_start_time and currentTime <= op.vis_end_time);quisp/channels/FSChannel.cc line 66 at r1 (raw file):
void FSChannel::recalculateChannelParameters() { par("distance").setDoubleValue(dist_par->getPropertyAtTime(simTime().dbl()));
accessing through par function is high cost operation. it's better to avoid this.
quisp/channels/QuantumChannel_FS.cc line 118 at r1 (raw file):
Dt = par("transmitter_telescope_diameter"); Dr = par("receiver_telescope_diameter"); r0 = par("Fried_parameter");
read these values in initialize
Code quote:
lambda = par("wavelength");
Dt = par("transmitter_telescope_diameter");
Dr = par("receiver_telescope_diameter");
r0 = par("Fried_parameter");0658996 to
c35ea43
Compare
…d variable latency
naming needs to be fixed: getTravelTimeLocal should be getCurrentTravelTime.
…lows routing decision even when satellite nodes start non-visible.
which was lost while cherry-picking
3d688e8 to
feb0f59
Compare
|
Hello there! I am available to work on these PRs and eventually merge them if someone could review them! |
This PR adds support for free space channels that will be useful for simulation of satellite networks.
This change is