AP_RangeFinder: Benewake-TFminiP driver outputs init failure to ground station #13300

ashvath100 · 2020-01-16T17:48:31Z

Fixes #12396
checks the version of the lidar and if it's too low, outputs a text string to the user.

peterbarker

This is a good find - thanks!

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp

rmackay9 · 2020-01-17T00:07:18Z

I wonder if the message will be visible on the ground station because the init will happen very early on in the startup. I suspect it will appear but it would be good to confirm.

rmackay9 · 2020-01-17T01:18:33Z

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp

@@ -102,6 +104,8 @@ bool AP_RangeFinder_Benewake_TFMiniPlus::init()
    if (val[5] * 10000 + val[4] * 100 + val[3] < 20003) {
        hal.console->printf(DRIVER ": minimum required FW version 2.0.3, but version %u.%u.%u found\n",
                            val[5], val[4], val[3]);
+        gcs().send_text(MAV_SEVERITY_CRITICAL, ": minimum required FW version 2.0.3, but version %u.%u.%u found",


It's funny that there is nothing before the ":". In the printf it has DRIVER. I also think this line is too long for a send-text which has a max length of 50 characters. Maybe try making it short like, "BenewakeTFMini: FW ver %u.%u.%u (2.0.3 required)"

Does "TFMini: FW ver %u.%u.%u (need>=2.0.3)" sound fine ?

Yes, that's fine I think.

The correction has been done

peterbarker · 2020-01-17T03:02:06Z

On Thu, 16 Jan 2020, Randy Mackay wrote: 50 characters. Maybe try making it short like, "BenewakeTFMini: FW ver %u.%u.%u (2.0.3 required)"

"BenewakeTFMini: FW ver %u.%u.%u (need>=2.0.3) Do we need "Benewake" in there?

peterbarker · 2020-01-19T02:34:52Z

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp

@@ -102,6 +104,8 @@ bool AP_RangeFinder_Benewake_TFMiniPlus::init()
    if (val[5] * 10000 + val[4] * 100 + val[3] < 20003) {
        hal.console->printf(DRIVER ": minimum required FW version 2.0.3, but version %u.%u.%u found\n",
                            val[5], val[4], val[3]);
+        gcs().send_text(MAV_SEVERITY_CRITICAL, "TFMini: FW ver %u.%u.%u (need>=2.0.3)",


This is being called with a semaphore held. This is actually really not a good idea - we want to cross as little code as possible when holding a semaphore - and gcs().send_text() isn't it :-)

Could you stash the three values in some variables and detect the version problem outside the semaphore-held part, please?

peterbarker · 2020-01-19T02:35:04Z

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp

@@ -102,6 +104,8 @@ bool AP_RangeFinder_Benewake_TFMiniPlus::init()
    if (val[5] * 10000 + val[4] * 100 + val[3] < 20003) {
        hal.console->printf(DRIVER ": minimum required FW version 2.0.3, but version %u.%u.%u found\n",


We can do without this once we have the send_text.

We still need to check the version here right ? since it decides if we goto fail or not

peterbarker · 2020-01-19T02:36:41Z

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp

@@ -102,6 +104,8 @@ bool AP_RangeFinder_Benewake_TFMiniPlus::init()
    if (val[5] * 10000 + val[4] * 100 + val[3] < 20003) {
        hal.console->printf(DRIVER ": minimum required FW version 2.0.3, but version %u.%u.%u found\n",
                            val[5], val[4], val[3]);
+        gcs().send_text(MAV_SEVERITY_CRITICAL, "TFMini: FW ver %u.%u.%u (need>=2.0.3)",


Oh, also, "CRITICAL" might going a bit far :-)

How about MAV_SEVERITY_ERROR?

I was the one who actually suggested MAV_SEVERITY_CRITICAL so let's not give @ashvath100 too much run around.

lucasdemarchi · 2020-01-20T18:55:30Z

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp

+    if (ver[0] * 10000 + ver[1] * 100 + ver[3] < 20003) {
+        gcs().send_text(MAV_SEVERITY_ERROR, "TFMini: FW ver %u.%u.%u (need>=2.0.3)",
+                            (unsigned)ver[0],(unsigned)ver[1],(unsigned)ver[2]);
+    }   


this code will never execute since you have the return false just above this

and you are checking the version again. I disagree with @peterbarker here: the failure is just on init and the additional time we hold the semaphore for this error condition doesn't matter much. I'd say to either move it where it was or change it in such a way you don't check the version twice.

rmackay9 · 2020-01-21T02:18:00Z

I guess you're going to open another PR @ashvath100? Sorry for the difficulties in getting this into master but you're almost there. Tell me if you'd prefer I just take your changes and modify them enough to get them merged.

ashvath100 · 2020-01-21T03:30:49Z

I guess you're going to open another PR @ashvath100? Sorry for the difficulties in getting this into master but you're almost there. Tell me if you'd prefer I just take your changes and modify them enough to get them merged.

@rmackay9 I'll be happy to finish the work , could you please clarify me on the semaphore issue whether or not the send_text() should be moved else where , if yes how do you propose we move it considering we will have to check the version again if we don't want to call any functions through the existing check

@peterbarker had suggested to use severity ERROR while you suggest to use CRITICAL , which should we finally use

…ound station

rmackay9 · 2020-01-22T02:22:37Z

@ashvath100,

MAV_SEVERITY_ERROR is probably best. Sorry for the confusion. The important thing for me is that it appears on the Mission Planner HUD in red letters... it should if sent as MAV_SEVERITY_ERROR (or _CRITICAL).

I'm not sure where the send-text should be placed in the code.

tatsuy · 2020-12-21T01:33:40Z

this was merged by #16102.
Please close this if that's OK with you.

peterbarker · 2020-12-21T01:44:23Z

@ashvath100 - you reopened?

ashvath100 changed the title ~~AP_RangeFinder: Benewake-TFminiP driver output init failure to ground station~~ AP_RangeFinder: Benewake-TFminiP driver outputs init failure to ground station Jan 16, 2020

ashvath100 requested a review from peterbarker January 16, 2020 18:42

peterbarker requested changes Jan 16, 2020

View reviewed changes

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp Outdated Show resolved Hide resolved

libraries/AP_RangeFinder/AP_RangeFinder_Benewake_TFMiniPlus.cpp Outdated Show resolved Hide resolved

ashvath100 force-pushed the patch branch from f244819 to 9985927 Compare January 16, 2020 22:55

ashvath100 force-pushed the patch branch from 9985927 to ff49cb5 Compare January 17, 2020 00:21

rmackay9 reviewed Jan 17, 2020

View reviewed changes

ashvath100 force-pushed the patch branch from ff49cb5 to 9dfc751 Compare January 17, 2020 11:46

ashvath100 requested review from peterbarker and rmackay9 January 17, 2020 18:19

peterbarker requested changes Jan 19, 2020

View reviewed changes

peterbarker reviewed Jan 19, 2020

View reviewed changes

ashvath100 force-pushed the patch branch 2 times, most recently from fdd9f62 to 5930813 Compare January 20, 2020 17:49

lucasdemarchi reviewed Jan 20, 2020

View reviewed changes

ashvath100 force-pushed the patch branch from 5930813 to cea9602 Compare January 20, 2020 19:06

ashvath100 closed this Jan 20, 2020

ashvath100 reopened this Jan 20, 2020

ashvath100 force-pushed the patch branch from cea9602 to bbe8766 Compare January 20, 2020 19:40

ashvath100 closed this Jan 20, 2020

ashvath100 reopened this Jan 21, 2020

AP_RangeFinder: Benewake-TFminiPlus driver outputs init failure to gr…

2ec86dc

…ound station

ashvath100 force-pushed the patch branch from bbe8766 to 2ec86dc Compare January 21, 2020 14:21

Merge branch 'master' into patch

46c5382

tatsuy mentioned this pull request Dec 19, 2020

AP_RangeFinder: TFminiPlus: change the frequency setting #16102

Merged

tatsuy mentioned this pull request Dec 21, 2020

AP_RangeFinder: Benewake-TFminiPlus driver should output init failure to ground station #12396

Closed

ashvath100 closed this Dec 21, 2020

ashvath100 reopened this Dec 21, 2020

ashvath100 closed this Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_RangeFinder: Benewake-TFminiP driver outputs init failure to ground station #13300

AP_RangeFinder: Benewake-TFminiP driver outputs init failure to ground station #13300

ashvath100 commented Jan 16, 2020

peterbarker left a comment

rmackay9 commented Jan 17, 2020

rmackay9 Jan 17, 2020

ashvath100 Jan 17, 2020

rmackay9 Jan 17, 2020

ashvath100 Jan 17, 2020

peterbarker commented Jan 17, 2020 via email

peterbarker Jan 19, 2020

peterbarker Jan 19, 2020

ashvath100 Jan 20, 2020

peterbarker Jan 19, 2020

rmackay9 Jan 21, 2020

lucasdemarchi Jan 20, 2020

lucasdemarchi Jan 20, 2020

rmackay9 commented Jan 21, 2020

ashvath100 commented Jan 21, 2020 •

edited

rmackay9 commented Jan 22, 2020

tatsuy commented Dec 21, 2020

peterbarker commented Dec 21, 2020

		@@ -102,6 +104,8 @@ bool AP_RangeFinder_Benewake_TFMiniPlus::init()
		if (val[5] * 10000 + val[4] * 100 + val[3] < 20003) {
		hal.console->printf(DRIVER ": minimum required FW version 2.0.3, but version %u.%u.%u found\n",

AP_RangeFinder: Benewake-TFminiP driver outputs init failure to ground station #13300

AP_RangeFinder: Benewake-TFminiP driver outputs init failure to ground station #13300

Conversation

ashvath100 commented Jan 16, 2020

peterbarker left a comment

Choose a reason for hiding this comment

rmackay9 commented Jan 17, 2020

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

peterbarker commented Jan 17, 2020 via email

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

rmackay9 commented Jan 21, 2020

ashvath100 commented Jan 21, 2020 • edited

rmackay9 commented Jan 22, 2020

tatsuy commented Dec 21, 2020

peterbarker commented Dec 21, 2020

ashvath100 commented Jan 21, 2020 •

edited