Wifi positioning #6
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ToppDev
requested changes
Jul 30, 2024
There was a problem hiding this comment.
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.
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.
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.
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.
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.
All references that need updating
There was a problem hiding this comment.
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.
Yes, creating a custom struct sounds like a good solution
Comment on lines
+124
to
+138
| 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.
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.
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.
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.
Suggested change
| void NAV::UartPacketConverter::receiveObs(NAV::InputPin::NodeDataQueue& queue, [[maybe_unused]] size_t pinIdx) | |
| void NAV::UartPacketConverter::receiveObs(NAV::InputPin::NodeDataQueue& queue, size_t /* pinIdx */) |
Comment on lines
+183
to
+184
| std::shared_ptr<NodeData> | ||
| convertedData = nullptr; |
| { | ||
| auto uartPacket = std::static_pointer_cast<const UartPacket>(queue.extract_front()); | ||
| // auto timeSyncMaster = std::static_pointer_cast<const TimeSync>(queue.extract_front()); |
Co-authored-by: ToppDev <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.