-
Notifications
You must be signed in to change notification settings - Fork 10
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
Wifi positioning #6
base: develop
Are you sure you want to change the base?
Wifi positioning #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started reviewing some things, but then stopped. You need to clean up the PR, otherwise I have to leave a comment every second line.
Also try testing the GUI elements, e.g. the UartPacketConverter
node is now completely broken with input pin when switching back to other types.
I think from the review items you get a general idea of what we expect. Please clean up the PR and request another review afterwards.
Also output from our internal pipelines
Documentation
/builds/instinct/instinct/src/NodeData/WiFi/WiFiPositioningSolution.hpp(22): error: Compound NAV::WiFiPositioningSolution is not documented.
/builds/instinct/instinct/src/util/Vendor/Espressif/EspressifUtilities.hpp(28): error: The following parameter of NAV::vendor::espressif::decryptWiFiObs(const std::shared_ptr< NAV::WiFiObs > &obs, uart::protocol::Packet &packet, const std::string &nameId) is not documented:
parameter 'nameId'
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.hpp(86): error: Member _sshHostkeys (variable) of class NAV::ArubaSensor is not documented.
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.hpp(87): error: Member _sshKeyExchange (variable) of class NAV::ArubaSensor is not documented.
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.hpp(85): error: Member _sshPassword (variable) of class NAV::ArubaSensor is not documented.
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.hpp(88): error: Member _sshPublickeyAcceptedTypes (variable) of class NAV::ArubaSensor is not documented.
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.hpp(84): error: Member _sshUser (variable) of class NAV::ArubaSensor is not documented.
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.hpp(160): error: Member distance (variable) of struct NAV::WiFiPositioning::Device is not documented.
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.hpp(161): error: Member distanceStd (variable) of struct NAV::WiFiPositioning::Device is not documented.
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.hpp(158): error: Member position (variable) of struct NAV::WiFiPositioning::Device is not documented.
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.hpp(159): error: Member time (variable) of struct NAV::WiFiPositioning::Device is not documented.
Clang-tidy: UartPacketConverter.cpp
/builds/instinct/instinct/src/NodeData/WiFi/WiFiObs.hpp:22:7: error: constructor does not initialize these fields: distance, distanceStd [cppcoreguidelines-pro-type-member-init,hicpp-member-init,-warnings-as-errors]
class WiFiObs : public NodeData
^
36858 warnings generated.
Suppressed 36970 warnings (36856 in non-user code, 114 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
Clang-tidy: WiFiPositioning.cpp
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:37:1: error: constructor does not initialize these fields: _numOfDevices [cppcoreguidelines-pro-type-member-init,hicpp-member-init,-warnings-as-errors]
NAV::WiFiPositioning::WiFiPositioning()
^
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:163:85: error: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer,-warnings-as-errors]
if (ImGui::InputDouble(fmt::format("##InputX{}", rowIndex).c_str(), &_devicePositions.at(rowIndex)[0], 0.0, 0.0, "%.4fm"))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
_devicePositions.at(rowIndex).data()
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:184:88: error: 'data' should be used for accessing the data pointer instead of taking the address of the 0-th element [readability-container-data-pointer,-warnings-as-errors]
if (ImGui::InputDoubleL(fmt::format("##InputLat{}", rowIndex).c_str(), &_devicePositions.at(rowIndex)[0], -180, 180, 0.0, 0.0, "%.8f°"))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
_devicePositions.at(rowIndex).data()
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:216:95: error: floating point literal has suffix 'f', which is not uppercase [hicpp-uppercase-literal-suffix,readability-uppercase-literal-suffix,-warnings-as-errors]
if (ImGui::Button(fmt::format("Add Device##{}", size_t(id)).c_str(), ImVec2(columnWidth * 2.1f, 0)))
^ ~
F
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:219:29: error: use emplace_back instead of push_back [hicpp-use-emplace,modernize-use-emplace,-warnings-as-errors]
_deviceMacAddresses.push_back("00:00:00:00:00:00");
^~~~~~~~~~
emplace_back(
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:220:26: error: use emplace_back instead of push_back [hicpp-use-emplace,modernize-use-emplace,-warnings-as-errors]
_devicePositions.push_back(Eigen::Vector3d::Zero());
^~~~~~~~~~
emplace_back(
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:226:98: error: floating point literal has suffix 'f', which is not uppercase [hicpp-uppercase-literal-suffix,readability-uppercase-literal-suffix,-warnings-as-errors]
if (ImGui::Button(fmt::format("Delete Device##{}", size_t(id)).c_str(), ImVec2(columnWidth * 2.1f, 0)))
^ ~
F
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:785:9: error: use auto when initializing with a cast to avoid duplicating the type name [hicpp-use-auto,modernize-use-auto,-warnings-as-errors]
size_t index = static_cast<size_t>(std::distance(_deviceMacAddresses.begin(), it));
^~~~~~
auto
/builds/instinct/instinct/src/Nodes/DataProcessor/WiFi/WiFiPositioning.cpp:882:5: error: do not use 'else' after 'return' [llvm-else-after-return,readability-else-after-return,-warnings-as-errors]
else
^~~~
114711 warnings generated.
Suppressed 115007 warnings (114695 in non-user code, 312 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
9 warnings treated as errors
Clang-tidy: ArubaSensor.cpp
/builds/instinct/instinct/src/NodeData/WiFi/WiFiObs.hpp:22:7: error: constructor does not initialize these fields: distance, distanceStd [cppcoreguidelines-pro-type-member-init,hicpp-member-init,-warnings-as-errors]
class WiFiObs : public NodeData
^
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:26:1: error: constructor does not initialize these fields: _channel, _session [cppcoreguidelines-pro-type-member-init,hicpp-member-init,-warnings-as-errors]
NAV::ArubaSensor::ArubaSensor()
^
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:35:5: error: '_sshHost' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
_sshHost = "192.168.178.45";
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:36:5: error: '_sshUser' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
_sshUser = "admin";
^~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:37:5: error: '_sshPassword' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
_sshPassword = "admin1";
^~~~~~~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:38:5: error: '_sshHostkeys' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
_sshHostkeys = "ssh-rsa";
^~~~~~~~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:39:5: error: '_sshKeyExchange' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
_sshKeyExchange = "ecdh-sha2-nistp256";
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:40:5: error: '_sshPublickeyAcceptedTypes' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
_sshPublickeyAcceptedTypes = "ssh-rsa";
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:41:5: error: '_outputInterval' should be initialized in an in-class default member initializer [cppcoreguidelines-prefer-member-initializer,-warnings-as-errors]
_outputInterval = 3000;
^~~~~~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:175:21: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
if (_session == NULL)
^~~~
nullptr
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:198:41: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
if (ssh_userauth_password(_session, NULL, _sshPassword.c_str()) != SSH_AUTH_SUCCESS)
^~~~
nullptr
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:209:21: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
if (_channel == NULL)
^~~~
nullptr
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:284:5: error: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays,-warnings-as-errors]
char buffer[1024];
^
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:286:12: error: variable 'bytesRead' is not initialized [cppcoreguidelines-init-variables,-warnings-as-errors]
size_t bytesRead;
^
= 0
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:304:84: error: statement should be inside braces [hicpp-braces-around-statements,readability-braces-around-statements,-warnings-as-errors]
while (std::getline(iss, line) && line.find("Peer-bssid") == std::string::npos); // Skip lines until the header "Peer-bssid" is found
^
{
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:320:9: error: multiple declarations in a single statement reduces readability [readability-isolate-declaration,-warnings-as-errors]
int rtt, rssi, stdValue;
^~~~~~~~~~~~~~~~~~~~~~~~
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:320:13: error: variable 'rtt' is not initialized [cppcoreguidelines-init-variables,-warnings-as-errors]
int rtt, rssi, stdValue;
^
note: this fix will not be applied because it overlaps with another fix
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:320:18: error: variable 'rssi' is not initialized [cppcoreguidelines-init-variables,-warnings-as-errors]
int rtt, rssi, stdValue;
^
note: this fix will not be applied because it overlaps with another fix
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:320:24: error: variable 'stdValue' is not initialized [cppcoreguidelines-init-variables,-warnings-as-errors]
int rtt, rssi, stdValue;
^
note: this fix will not be applied because it overlaps with another fix
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:323:30: error: escaped string literal can be written as a raw string literal [modernize-raw-string-literal,-warnings-as-errors]
std::regex timeRegex("\\d{4}-\\d{2}-\\d{2}"); // Time format: YYYY-MM-DD
^~~~~~~~~~~~~~~~~~~~~~
R"(\d{4}-\d{2}-\d{2})"
/builds/instinct/instinct/src/Nodes/DataProvider/WiFi/Sensors/ArubaSensor.cpp:324:9: error: multiple declarations in a single statement reduces readability [readability-isolate-declaration,-warnings-as-errors]
std::string timeStamp1, timeStamp2;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
33858 warnings generated.
Suppressed 33931 warnings (33832 in non-user code, 99 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
21 warnings treated as errors
Clang-tidy: EspressifUtilities.cpp
/builds/instinct/instinct/src/util/Vendor/Espressif/EspressifUtilities.cpp:14:10: error: inclusion of deprecated C++ header 'string.h'; consider using 'cstring' instead [hicpp-deprecated-headers,modernize-deprecated-headers,-warnings-as-errors]
#include <string.h>
^~~~~~~~~~
<cstring>
33272 warnings generated.
Suppressed 33358 warnings (33270 in non-user code, 88 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
@@ -37,6 +37,7 @@ Read the docs on | |||
|
|||
##### Build & run the main program | |||
```shell | |||
sudo apt-get install libssh-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you did not use https://conan.io/center/recipes/libssh?
I tested the conan version in our internal pipeline and it seems to work on Windows, MacOs and Linux
cmake/modules/FindLibSSH.cmake
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually never used. Cmake is case sensitive, so when calling find_package(libssh REQUIRED)
, Cmake only considers a file Findlibssh.cmake
.
But if we use conan for the library, we can remove this file entirely.
@@ -1,6 +1,6 @@ | |||
[requires] | |||
spdlog/1.13.0 | |||
fmt/10.2.1 | |||
fmt/[<=10.2.1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change does not seem necessary in our internal pipelines
/// Standard deviation of the distance | ||
double distanceStd; | ||
/// Time of observation | ||
NAV::vendor::vectornav::TimeOutputs timeOutputs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason you hooking in some structure from the VectorNav here?
Should be avoided if possible, as we that way limiting the WiFi ranging to this specific manufacturer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we only need these two struct members, if so I could create a new custom struct with both parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, creating a custom struct sounds like a good solution
private: | ||
/// Standard deviation of Position in ECEF coordinates [m] | ||
Eigen::Vector3d _e_positionStdev = Eigen::Vector3d::Zero() * std::nan(""); | ||
/// Standard deviation of Position in local navigation frame coordinates [m] | ||
Eigen::Vector3d _n_positionStdev = Eigen::Vector3d::Zero() * std::nan(""); | ||
|
||
/// Standard deviation of Velocity in earth coordinates [m/s] | ||
Eigen::Vector3d _e_velocityStdev = Eigen::Vector3d::Zero() * std::nan(""); | ||
/// Standard deviation of Velocity in navigation coordinates [m/s] | ||
Eigen::Vector3d _n_velocityStdev = Eigen::Vector3d::Zero() * std::nan(""); | ||
|
||
/// Covariance matrix in ECEF coordinates (Position, Velocity) | ||
Eigen::MatrixXd _e_covarianceMatrix; | ||
/// Covariance matrix in local navigation coordinates (Position, Velocity) | ||
Eigen::MatrixXd _n_covarianceMatrix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unused, as it is included in the PosVel
parent class
{ | ||
LOG_DEBUG("{}: syncInPin changed to {}", nameId(), _syncInPin); | ||
flow::ApplyChanges(); | ||
if (_syncInPin && (inputPins.size() <= 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sync pin does not get removed when changing to a different output type afterwards
} | ||
} | ||
} | ||
if (j.contains("syncInPin")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should check for output type before adding a pin
@@ -123,11 +175,13 @@ bool NAV::UartPacketConverter::initialize() | |||
return true; | |||
} | |||
|
|||
void NAV::UartPacketConverter::receiveObs(NAV::InputPin::NodeDataQueue& queue, size_t /* pinIdx */) | |||
void NAV::UartPacketConverter::receiveObs(NAV::InputPin::NodeDataQueue& queue, [[maybe_unused]] size_t pinIdx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void NAV::UartPacketConverter::receiveObs(NAV::InputPin::NodeDataQueue& queue, [[maybe_unused]] size_t pinIdx) | |
void NAV::UartPacketConverter::receiveObs(NAV::InputPin::NodeDataQueue& queue, size_t /* pinIdx */) |
std::shared_ptr<NodeData> | ||
convertedData = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this
{ | ||
auto uartPacket = std::static_pointer_cast<const UartPacket>(queue.extract_front()); | ||
// auto timeSyncMaster = std::static_pointer_cast<const TimeSync>(queue.extract_front()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unused code
Co-authored-by: ToppDev <[email protected]>
No description provided.