Aerobridge Guardian (Version 0.2) by 1AwesomeDeveloper · Pull Request #12 · openutm-labs/qgroundcontrol

1AwesomeDeveloper · 2022-01-23T12:46:42Z

Features Implemented

Upload Plan File ( KML ) to Management Server
Create Operation Functionality
Select and Load Plan Functionality
Ask Management Server for Flight Permission
Verify the signature (JWT Verification)

not-vibhu · 2022-01-23T12:56:54Z

Is it possible to add OpenSSL as an external submodule? from here ?

botmayank

@1AwesomeDeveloper quite a lot of really nice work, have a look to see if the comments make sense.

@hrishiballal @not-vibhu @rhythmize

botmayank · 2022-03-18T18:58:26Z

modifications/ConnectToServer.h

@@ -4,18 +4,14 @@
 #include <QObject>
 #include "QGCApplication.h"

-///This class establishes a connection to the authentication server. If connection is succesful, it emits signal for the UI


@1AwesomeDeveloper do you want to leave at least one basic top level comment for the header?

botmayank · 2022-03-18T18:59:40Z

modifications/DataClass.cc

+}
+
+//API CALLS START
+//*******************************************************************************************************************************************************************************


Seems unnecessary to have API calls start and this comment

botmayank · 2022-03-18T19:00:32Z

modifications/DataClass.cc

    }
 }
-
+//*******************************************************************************************************************************************************************************


Remove this too I think

botmayank · 2022-03-18T19:01:47Z

modifications/DataClass.cc

        emit tokenNotGenerated();
+        showMessage("Unable to Connect to Server", statusCode);


Minor but connect and server need not be in capital, also if 201 and 200 checks are to be performed elsewhere, might be nice to have them as a #define/enum on top. Would help avoid having magic numbers in the code even though these are well known status codes.

Done, error codes are already an enum, no need to define in a separate header file because all API calls are made using dataClass

botmayank · 2022-03-18T19:01:51Z

modifications/DataClass.cc

    }
 }
-
+//*******************************************************************************************************************************************************************************


Remove this too I think

botmayank · 2022-03-18T19:17:51Z

modifications/NpntProcess.qml

            Column{
                    CheckBox {
                        id: c1
                        checked:myrect.hardwareConnected
-                        text: qsTr("Hardware Connected")
+                        text: qsTr("Connecting to the Flight Controller")


Connecting or connected?

Both are fine, the status is represented by a checkbox. The text was given by @not-vibhu.
Currently changing it to "connected". Can be reverted if needed

botmayank · 2022-03-18T19:18:47Z

modifications/SignatureVerifier.cc

+    EVP_PKEY * pRsaKey = EVP_PKEY_new();
+    RSA_set0_key(rr, modul, expon , kids);
+    EVP_PKEY_assign_RSA(pRsaKey, rr);
+    unsigned char * ss = new unsigned char[1024];


any way of doing this without new delete and using a unique_ptr?

@rhythmize

A follow up question: Why are we writing this SignatureVerifier::convertJwkToPem conversion ourselves and not using jwt-cpp approach since we're already using jwt-cpp?
Example: https://github.com/Thalhammer/jwt-cpp/blob/master/example/jwks-verify.cpp
Reference code: https://github.com/Thalhammer/jwt-cpp/blob/142d2d38e7bb1ad17b9787d776589e719d7494cb/include/jwt-cpp/jwt.h#L462

botmayank · 2022-03-18T19:19:12Z

modifications/SignatureVerifier.cc

+{         
+    std::error_code c;
+    try {
+        auto verify = jwt::verify().allow_algorithm(jwt::algorithm::rs256(strPubKey)).with_issuer("https://id.openskies.sh/");


Why is this URL not coming from the globals dict?

botmayank · 2022-03-18T19:20:44Z

src/MissionManager/PlanMasterController.cc

+QStringList PlanMasterController::getAllPlans() const
+{
+      QStringList allPlans = qgcApp()->getDataClass()->getAllPlans();
+      allPlans.push_front("No Plan Selected");


In general in your logs, avoid unnecessary capitalization of common words. Like this one should be "No plan selected"

botmayank · 2022-03-18T19:22:03Z

src/MissionManager/PlanMasterController.h

@@ -155,5 +186,7 @@ private slots:
    QmlObjectListModel*     _planCreators =             nullptr;
    DataClass* _dataClass;
    QString m_url;
+    int m_planIndex = 0;


How many plans are we going to be holding? Sure int is enough right?

Might make sense to have a larger data type if you need, or even with int, have a max bound for how many plans one is allowed to create/keep, and have a check to either delete older plans or overwrite in a circular fashion or alteast alert the operator.

@hrishiballal @rhythmize

I'd suggest to atleast make it unsigned int. It'll double the current limit and then we can have some checks to prevent any rollovers.

Changed it to unsigned long long int

kshitizgoyal5 and others added 8 commits December 2, 2021 20:16

added prompt for plan name input dialog for uploading plan in QML GUI

1ecc12d

Added fetch_all_flight_plans api in backend

aa82408

added flight permission apis

12ad925

Version 0.2 in progress

7bb2415

Added Plan Select Menu

cc996ce

Added OpenSSL support + v0.2 development

7b8472c

Version 0.2 Enhancements

85a159e

Added toast notifications, error handellings, refined code

f4924bf

hrishiballal mentioned this pull request Feb 9, 2022

Added toast notifications feature #11

Closed

botmayank reviewed Mar 18, 2022

View reviewed changes

1AwesomeDeveloper added 2 commits April 3, 2022 21:13

Fixed the build issue

1d4fb61

Changed after PR review

2248048

		emit tokenNotGenerated();
		showMessage("Unable to Connect to Server", statusCode);

Conversation

1AwesomeDeveloper commented Jan 23, 2022

Uh oh!

not-vibhu commented Jan 23, 2022

Uh oh!

botmayank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhythmize Mar 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rhythmize Mar 19, 2022 •

edited

Loading