Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AUV error management #9

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from
Draft

Conversation

alessiaortile
Copy link

No description provided.

Copy link
Contributor

@campagn1 campagn1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Some corrections need to be performed: I commented only one module, the suggestions apply to all.
Brava!


lib_LTLIBRARIES = libuwauv.la

libuwauv_la_SOURCES = uwauv-module.cc uwauv-module.h uwauverror-module.cc uwauverror-module.h uwauverror-b-module.cc uwauverror-b-module.h uwauv-packet.h uwauvctr-module.cc uwauvctr-module.h uwauvctrer-module.cc uwauvctrer-module.h uwauvctrer-b-module.cc uwauvctrer-b-module.h initlib.cc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine, but in principle be aware that only the cc (or cpp) files need to be listed

{
UWSMEPosition p = UWSMEPosition();
posit=&p;
bind("ackTimeout_", (int*) &ackTimeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(int*) or (double*)?

, out_file_stats(0)
{
posit = p;
bind("ackTimeout_", (int*) &ackTimeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int or double?

else if(argc == 3){
if (strcasecmp(argv[1], "setPosition") == 0) {
UWSMEPosition* p = dynamic_cast<UWSMEPosition*> (tcl.lookup(argv[2]));
posit=p;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if cast succeded with
if(p) {posit = p; ...}
if it did not, print that it failed and return TCL_ERR

if (atof(argv[2]) == 3) {
ackPolicy = ACK_PGBK_OR_TO;
return TCL_OK;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add else {cerr<< "Policy not supported" << std::endl; return TCL_ERR;}

**/
virtual int command(int argc, const char *const *argv);

virtual void setdest(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better doxygen (we will make it for smposition as well)

virtual void setdest(
double x_dest, double y_dest, double z_dest, double spead);

virtual void adddest(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addDest


virtual void adddest(double x_dest, double y_dest, double z_dest);

virtual void setAlarm(bool alarm);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alarm for what? Add doxygen

protected:
/**
* Method that updates both the position coordinates
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doxygen @param now ..


private:

std::vector<std::vector<double>> waypoints;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doxygen with
variable; /**< description here */

Copy link
Contributor

@campagn1 campagn1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the intentation as in the old comment

#define HDR_UWAUV_MONITORING(p) (hdr_uwAUV_monitoring::access(p))
#define HDR_UWAUV_CTR(p) (hdr_uwAUV_ctr::access(p))

using namespace std;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, remove "using namespace std;"

@@ -0,0 +1,51 @@
#
# Copyright (c) 2007 Regents of the SIGNET lab, University of Padova.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to begin to be consistent on this, so i would put 2023 here. I know we never updated it, but we should start

DESERT_Addons/uwauv/m4/desert.m4 Show resolved Hide resolved
, ackPriority(0)
, ackNotPgbk(0)
, drop_old_waypoints(0)
, log_flag(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call it sth more consisent, like "log_on_file", this way is more clear for what is this flag (we all know it's a flag)

ackPolicy = ACK_PGBK_OR_TO;
return TCL_OK;
}else{
cerr<<"Plicy not supported" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cerr<<"Plicy not supported" << std::endl;
cerr<<"Policy not supported" << std::endl;


void UwAUVModule::recv(Packet* p) {

hdr_uwAUV_ctr* uwAUVh = hdr_uwAUV_ctr::access(p);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are ALL the packets travelling in the network with this header for sure?

@@ -0,0 +1,188 @@
import csv
import matplotlib.pyplot as plt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not come with a standard python installation i guess. Did you installed a custom pip? if so, better write in a readme, so that someone else know in advance which deps he needs to be installed and how

@@ -0,0 +1,152 @@
import csv
import matplotlib.pyplot as plt
import tikzplotlib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as before

@@ -0,0 +1,173 @@
import csv
import matplotlib.pyplot as plt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@@ -0,0 +1,428 @@
import matplotlib.pyplot as plt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@@ -99,3 +99,66 @@
2361.830056,-99.802673,-48.175367,-15.000000
2391.136904,-99.950656,-24.868989,-15.000000
2422.005942,-100.000000,-0.000000,-15.000000
2622.000000,100.000000,0.000000,-15.000000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@campagn1 is this used in other simulations? is this ok with the other tcl or do we have to do some no-regression on other tcls?

@fedefava86 fedefava86 marked this pull request as draft September 21, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants