Pull Request for AHRS IO Processor #16

mfs16101 · 2020-05-08T18:27:59Z

This is basically a redo of the a previous pull request that had a bunch of old code in it still. It also fixes some comments left on that pull request.

grf14003

Matt,

It looks like you didn't see my inline comments from my last review. Make sure to address those (there is about 30 of them) before you update this request again.

Best regards and happy editing,
Greg

grf14003 · 2020-05-08T19:10:48Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+		for (int i=0; i < 8; i++)
+			yawArray[i] = szBuff[index + (8-i)];
+	}
+


We need more than just the yaw angle. We need all of them!

The other angles don't change if we are only in a 2D plane, but I should be able to get the others also.

grf14003 · 2020-05-08T19:11:03Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+
+bool AHRSInputIOProcessor::loopCondition()
+{
+    return iterations > 0;


You are still using this iterations variable for some reason.

I have removed the iterations thing, but without it I don't know when to close the handle.

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

There is a problem that may be cause by these changes, that being not closing the handle, since the loop will run forever, I would have to close the loop on some sort of exit condition. I am not sure if the handle closes if the process is killed, if so, then I guess it should be fine.

mfs16101 · 2020-05-08T19:51:25Z

~~Okay, I'm not sure exactly how to update this pull request with my new commit.~~ It does it automatically. Most of the things you commented on are changed. Except for where you said to make things "constant" what do you mean by that? Like declare them as private variables or as "#define"? I didn't make the COM port change since I am unsure of how exactly that would work and I fear it would break the code completely, I will look further into it. Also, there may be a problem with closing the handle since the loop runs forever. If the handle automatically closes on a process kill then it shouldn't matter.

mfs16101 · 2020-05-08T20:02:23Z

I just realized I did not rebuild with the python script before pushing, I will do that with my next commit.

mfs16101 · 2020-05-08T20:22:46Z

Welp, now I get a bunch of red lines when I try and build... It might have to do with the Untabify but I don't see why it would. All the other changes were mostly cosmetic, except for changing the couts to cerrs though I don't see how that could have caused build errors either...

mfs16101 · 2020-05-08T20:25:07Z

All the errors seem to be from something other than the code itself. Though that might just be me not understanding how the python script works.

This reverts commit 5a9cb08.

This reverts commit e2ea108.

I have no idea why exactly but the build errors only got fixed when I changed the order of the includes in AHRSInputIOProcessor.hpp.

grf14003

Hey Matt,

Just a couple more little things to change.

Best regards,
Greg

grf14003 · 2020-05-08T23:30:50Z

bfs/implementations/breadcrumbs/include/AHRSInputIOProcessor.hpp

+#ifndef AHRS_INPUT_IO_PROCESSOR_HPP
+#define AHRS_INPUT_IO_PROCESSOR_HPP
+
+


Too many spaces here, there should be only one

grf14003 · 2020-05-08T23:31:14Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+ * 7 : Error occured while trying to send SetOutputConfiguration message.
+ * 8 : Error occured while trying to send GoToMeasurment message.
+*/
+


Get rid of this space so this comment is sitting right on top of the corresponding function

grf14003 · 2020-05-08T23:36:07Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+    {
+        for (int i = 0; i < 8; i++)
+        {
+            rollArray[i] = szBuff[index - (i + 8)];


This is really confusing. It would be better if index is first of all named something better like dataIndexStart, and second of all holding the value of the first piece of data rather than the first yaw index. That way all the bits will be possitively offset from the index variable rather than this confusing arithmetic.

There will be a bit of confusing arithmetic any way I do it because I'm looping backwards, but hopefully the new version is a bit more clear.

grf14003 · 2020-05-08T23:36:18Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+    {
+        if (szBuff[i] == 0xFA && szBuff[i+1] == 0xFF && szBuff[i+2] == 0x36)
+        {
+            index = i + 22;


Add a comment here for why this is + 22

grf14003 · 2020-05-08T23:37:44Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+    double doubleRoll = *reinterpret_cast<double* const>(rollArray);
+    double doublePitch = *reinterpret_cast<double* const>(pitchArray);
+    double doubleYaw = *reinterpret_cast<double * const>(yawArray);
+    Attribute attRoll("rolAngle", 8, &doubleRoll);


These abbreviations should be all caps and reference the manuafacturer and the type of device they came from... maybe "XSIMUROL", etc.

grf14003 · 2020-05-08T23:38:15Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+    getComms()->sendAttribute(attYaw);
+
+    // I am not sure if it will close autimatically when the process ends, at the moment I guess we just won't close it...
+    //CloseHandle(hSerial);


Get rid of the commented out code. It will close when the process ends

I added some comments explaining how I am capturing the bytes of the Euler Angles from the message in the buffer. Quick version here, I first capute a 2 times the size of the message into a buffer, I then search for the begining of the message, when I find it, I then jump to the index where the Angles are stored. I then add each group of 8 bytes for each angle backwords into arrays to then be converted. I add them in backwords since the AHRS is big endian and the computer we are converting them to doubles on is little endian. After each angle's 8 bytes are storred in an array, I then cast them into doubles, where they can then be sent. Hopefully that makes sense, feel free to ask me any questions about it.

I fixed the math for getting the bytes for the Angles to make a bit easier to understand. I also clarfied my comments a bit to try and make them clearer with the explanation.

I further clarified my comments and also renamed index to dataStart to make it more clear what it was.

Changed "index" to "dataStart" in diagram to make it more clear.

grf14003

Matt,

Great job on this, there is just one little comment I left on how to make it slightly more intuitive. Make sure to address that before you merge but otherwise LGTM

Regards,
Greg

grf14003 · 2020-05-11T15:25:05Z

bfs/implementations/breadcrumbs/io_procs/AHRSInputIOProcessor.cpp

+    // Puts the bytes for each angle in their own array backwards, since they are sent in big endian
+    if (dataStart != -1)
+    {
+        for (int i = 0; i < 8; i++)


Instead of a forward for loop, wouldn't a reverese for loop be more intuitive: for (i = 7; i >= 0; i--)

In some ways yes, I could start at the actual start of the data, but there would still be some funky arithmetic going on.
It would be:
dataStart = i + 7;
for (int i = 7; i > -1; i--)
{
rollArray[i] = szBuff[dataStart + i];
pitchArray[i] = szBuff[dataStart + i + 8];
yawArray[i] = szBuff[dataStart + i + 16];
}

Does that actually make it clearer?

It just comes down to individual preference with the equations if they make more sense...

It changes the - and + into just two +'s which is a bit more clearer.

Changed the for loop to loop backwords itself so that the equations for actually getting the bytes only use +'s. They are still basically the same equations, but hopefully they are a little more intuitive...

mfs16101 added 3 commits May 8, 2020 13:45

Add AHRS IO Proc files into newer pull from dev

17bdc02

Fix names and built with python script

ac92532

Fix pull request comments

f3f13b8

mfs16101 added the enhancement New feature or request label May 8, 2020

mfs16101 requested a review from grf14003 May 8, 2020 18:27

mfs16101 self-assigned this May 8, 2020

grf14003 requested changes May 8, 2020

View reviewed changes

Untabify AHRSInputIOProcessor.cpp

3cbcf6e

mfs16101 added 4 commits May 8, 2020 16:26

Hmmm

5a9cb08

Revert "Hmmm"

e2ea108

This reverts commit 5a9cb08.

Add functionality to get roll and pitch angles

98e1ba6

This reverts commit e2ea108.

Change order of #includes fixed build errors...

277cb31

I have no idea why exactly but the build errors only got fixed when I changed the order of the includes in AHRSInputIOProcessor.hpp.

grf14003 requested changes May 8, 2020

View reviewed changes

mfs16101 and others added 6 commits May 10, 2020 17:08

Fix confusing math

6bb6f9c

I fixed the math for getting the bytes for the Angles to make a bit easier to understand. I also clarfied my comments a bit to try and make them clearer with the explanation.

Fix Comments More

45ae105

I further clarified my comments and also renamed index to dataStart to make it more clear what it was.

Rename Attribute Keys

f446d77

Fix type in comments

58a3dec

Changed "index" to "dataStart" in diagram to make it more clear.

Add more specifics to Config message comments

13c66ec

grf14003 approved these changes May 11, 2020

View reviewed changes

Change arround data byte capture math

9988615

Changed the for loop to loop backwords itself so that the equations for actually getting the bytes only use +'s. They are still basically the same equations, but hopefully they are a little more intuitive...

mfs16101 merged commit 64a5fc4 into dev May 11, 2020

Pull Request for AHRS IO Processor #16

Pull Request for AHRS IO Processor #16

mfs16101 commented May 8, 2020 •

edited

Loading

grf14003 left a comment

grf14003 May 8, 2020

mfs16101 May 8, 2020

grf14003 May 8, 2020

mfs16101 May 8, 2020

mfs16101 commented May 8, 2020 •

edited

Loading

mfs16101 commented May 8, 2020

mfs16101 commented May 8, 2020

mfs16101 commented May 8, 2020

grf14003 left a comment

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

mfs16101 May 10, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 May 8, 2020

grf14003 left a comment

grf14003 May 11, 2020

mfs16101 May 11, 2020

mfs16101 May 11, 2020

mfs16101 May 11, 2020

		#ifndef AHRS_INPUT_IO_PROCESSOR_HPP
		#define AHRS_INPUT_IO_PROCESSOR_HPP

Pull Request for AHRS IO Processor #16

Pull Request for AHRS IO Processor #16

Conversation

mfs16101 commented May 8, 2020 • edited Loading

grf14003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfs16101 commented May 8, 2020 • edited Loading

mfs16101 commented May 8, 2020

mfs16101 commented May 8, 2020

mfs16101 commented May 8, 2020

grf14003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grf14003 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfs16101 commented May 8, 2020 •

edited

Loading

mfs16101 commented May 8, 2020 •

edited

Loading