Rover: Log_Write_Nav_Tuning should log wp_bearing and nav_bearing in degrees #13162

rishabsingh3003 · 2020-01-03T18:00:06Z

This is in response to #11312.

magicrub

wrap_360_cd() needs to change to wrap_360()

magicrub · 2020-01-03T18:14:24Z

on a side note for another PR, I see that the wrap_360() and wrap_360_cd() functions start off with a mod function which is relatively expensive. We call those helper functions a lot so we could benefit from an optimization there. We should change that to check for being within 0 to 360 (or 0 to 36000) and exit immediately if within correct bounds.

rishabsingh3003 · 2020-01-03T18:25:34Z

wrap_360_cd() needs to change to wrap_360()

That was an embarrassing mistake! Sorry, it's done now!

magicrub · 2020-01-03T18:30:27Z

Doesn't this need to be changed for all vehicles? We don't want copter logs having different data than rover/plane/sub

magicrub · 2020-01-03T18:34:01Z

ahh, nevermind. I see copter doesn't have it and plane is already fixed. So this fixes it all up. Looks good!

magicrub · 2020-01-03T18:45:03Z

well, now that I think of it, this introduces other problems. The units and multipliers are wrong.

Check LogStructure.h

Both plane and Rover have units and multiplier as:

{ 'd', "deg" },           // of the angular variety, -180 to 180
{ 'B', 1e-2 },

Plane log definition:

    { LOG_NTUN_MSG, sizeof(log_Nav_Tuning),         
      "NTUN", "QfcccfffLLii",  "TimeUS,Dist,TBrg,NavBrg,AltErr,XT,XTi,AspdE,TLat,TLng,TAlt,TAspd", "smddmmmnDUmn", "F0BBB0B0GGBB" },

Rover log definition:

    { LOG_NTUN_MSG, sizeof(log_Nav_Tuning),
      "NTUN", "QfHHHf", "TimeUS,WpDist,WpBrg,DesYaw,Yaw,XTrack", "smdddm", "F0BBB0" },

So, if we are wanting 0-360 heading, we'll want unit='h' multiplier='0'. is that what we want? For something like this where two vehicles share a log entry it would be nice to at least have them match (and be correct).

rishabsingh3003 · 2020-01-03T19:47:43Z

@magicrub I changed the unit and multiplier for the rover. Hopefully, this is okay now?

peterbarker · 2020-01-06T03:40:37Z

on a side note for another PR, I see that the wrap_360() and wrap_360_cd() functions start off with a mod function which is relatively expensive. We call those helper functions a lot so we could benefit from an optimization there. We should change that to check for being within 0 to 360 (or 0 to 36000) and exit immediately if within correct bounds.

Good call.

peterbarker · 2020-01-07T03:28:46Z

on a side note for another PR, I see that the wrap_360() and wrap_360_cd() functions start off with a mod function which is relatively expensive. We call those helper functions a lot so we could benefit from an optimization there. We should change that to check for being within 0 to 360 (or 0 to 36000) and exit immediately if within correct bounds.

Good call.

... actually, I just measured. fly.ArduPlane.MainFlight only calls wrap_360 2,500 times - so it might not be worth it. More measurement would be required to see if it is.

peterbarker · 2020-01-07T03:34:59Z

APMrover2/Log.cpp

@@ -99,8 +99,8 @@ struct PACKED log_Nav_Tuning {
    LOG_PACKET_HEADER;
    uint64_t time_us;
    float wp_distance;
-    uint16_t wp_bearing_cd;
-    uint16_t nav_bearing_cd;
+    uint16_t wp_bearing;


Please make this float.

peterbarker · 2020-01-07T03:35:13Z

APMrover2/Log.cpp

@@ -112,8 +112,8 @@ void Rover::Log_Write_Nav_Tuning()
        LOG_PACKET_HEADER_INIT(LOG_NTUN_MSG),
        time_us             : AP_HAL::micros64(),
        wp_distance         : control_mode->get_distance_to_destination(),
-        wp_bearing_cd       : (uint16_t)wrap_360_cd(control_mode->wp_bearing() * 100),
-        nav_bearing_cd      : (uint16_t)wrap_360_cd(control_mode->nav_bearing() * 100),
+        wp_bearing          : (uint16_t)wrap_360(control_mode->wp_bearing()),


Remove the cast and the wrap_360

peterbarker · 2020-01-07T03:35:23Z

APMrover2/Log.cpp

@@ -252,7 +252,7 @@ const LogStructure Rover::log_structure[] = {
    { LOG_THR_MSG, sizeof(log_Throttle),
      "THR", "Qhffff", "TimeUS,ThrIn,ThrOut,DesSpeed,Speed,AccY", "s--nno", "F--000" },
    { LOG_NTUN_MSG, sizeof(log_Nav_Tuning),
-      "NTUN", "QfHHHf", "TimeUS,WpDist,WpBrg,DesYaw,Yaw,XTrack", "smdddm", "F0BBB0" },


Need ot say "float" here.

rishabsingh3003 · 2020-01-07T10:57:30Z

@peterbarker done! Please check and let me know if anything else is required This is my first time dealing with logs and was a bit confusing. But it makes perfect sense now :)

peterbarker · 2020-01-07T15:33:19Z

APMrover2/Log.cpp

@@ -112,8 +112,8 @@ void Rover::Log_Write_Nav_Tuning()
        LOG_PACKET_HEADER_INIT(LOG_NTUN_MSG),
        time_us             : AP_HAL::micros64(),
        wp_distance         : control_mode->get_distance_to_destination(),
-        wp_bearing_cd       : (uint16_t)wrap_360_cd(control_mode->wp_bearing() * 100),
-        nav_bearing_cd      : (uint16_t)wrap_360_cd(control_mode->nav_bearing() * 100),
+        wp_bearing          : wrap_360(control_mode->wp_bearing()),


Don't need the wrap_360 here. If it's not between 0 and 360 we have a problem :-)

peterbarker

I believe this now does what Randy requested.

rmackay9 · 2020-03-02T11:24:39Z

I gave this a quick test and it's looking good, thanks very much and sorry for the delay in merging.

rishabsingh3003 · 2020-03-02T12:20:43Z

@rmackay9 Thanks!

magicrub requested changes Jan 3, 2020

View reviewed changes

rishabsingh3003 force-pushed the rover_nav_tuning_log branch from db18133 to 526fd2c Compare January 3, 2020 18:24

rishabsingh3003 requested a review from magicrub January 3, 2020 18:25

rishabsingh3003 force-pushed the rover_nav_tuning_log branch from 526fd2c to 5ead1e3 Compare January 3, 2020 19:41

rishabsingh3003 closed this Jan 4, 2020

rishabsingh3003 reopened this Jan 4, 2020

peterbarker requested changes Jan 7, 2020

View reviewed changes

rishabsingh3003 force-pushed the rover_nav_tuning_log branch from 5ead1e3 to 5a5bf06 Compare January 7, 2020 06:18

rishabsingh3003 closed this Jan 7, 2020

rishabsingh3003 reopened this Jan 7, 2020

rishabsingh3003 requested a review from peterbarker January 7, 2020 10:55

peterbarker requested changes Jan 7, 2020

View reviewed changes

Rover: Changed logging of wp_bearing and nav_bearing to degrees

014737a

rishabsingh3003 force-pushed the rover_nav_tuning_log branch from 5a5bf06 to 014737a Compare January 7, 2020 16:02

rishabsingh3003 requested a review from peterbarker January 7, 2020 17:00

peterbarker approved these changes Jan 7, 2020

View reviewed changes

peterbarker requested a review from rmackay9 January 7, 2020 23:38

rmackay9 merged commit 0d5f511 into ArduPilot:master Mar 2, 2020

rishabsingh3003 mentioned this pull request Mar 4, 2020

Rover: Log_Write_Nav_Tuning should log wp_bearing and nav_bearing in degrees #11312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rover: Log_Write_Nav_Tuning should log wp_bearing and nav_bearing in degrees #13162

Rover: Log_Write_Nav_Tuning should log wp_bearing and nav_bearing in degrees #13162

rishabsingh3003 commented Jan 3, 2020

magicrub left a comment

magicrub commented Jan 3, 2020

rishabsingh3003 commented Jan 3, 2020

magicrub commented Jan 3, 2020

magicrub commented Jan 3, 2020

magicrub commented Jan 3, 2020

rishabsingh3003 commented Jan 3, 2020

peterbarker commented Jan 6, 2020

peterbarker commented Jan 7, 2020

peterbarker Jan 7, 2020

peterbarker Jan 7, 2020

peterbarker Jan 7, 2020

rishabsingh3003 commented Jan 7, 2020

peterbarker Jan 7, 2020

peterbarker left a comment

rmackay9 commented Mar 2, 2020

rishabsingh3003 commented Mar 2, 2020

Rover: Log_Write_Nav_Tuning should log wp_bearing and nav_bearing in degrees #13162

Rover: Log_Write_Nav_Tuning should log wp_bearing and nav_bearing in degrees #13162

Conversation

rishabsingh3003 commented Jan 3, 2020

magicrub left a comment

Choose a reason for hiding this comment

magicrub commented Jan 3, 2020

rishabsingh3003 commented Jan 3, 2020

magicrub commented Jan 3, 2020

magicrub commented Jan 3, 2020

magicrub commented Jan 3, 2020

rishabsingh3003 commented Jan 3, 2020

peterbarker commented Jan 6, 2020

peterbarker commented Jan 7, 2020

peterbarker Jan 7, 2020

Choose a reason for hiding this comment

peterbarker Jan 7, 2020

Choose a reason for hiding this comment

peterbarker Jan 7, 2020

Choose a reason for hiding this comment

rishabsingh3003 commented Jan 7, 2020

peterbarker Jan 7, 2020

Choose a reason for hiding this comment

peterbarker left a comment

Choose a reason for hiding this comment

rmackay9 commented Mar 2, 2020

rishabsingh3003 commented Mar 2, 2020