Pr tfminiplus and command support

[amoizard]

Describe problem solved by this pull request

• The first commit is fixing the FOV of TFMini model and removing an obsolete "tfmini test" command line description. Manual can be checked here: http://en.benewake.com/support_data?catename=03c54a91-8f9a-409f-985d-37acc68f7bf4&supportid=1324951b-e4d4-4594-9da9-fc76d72f98de&scroll=200
• The second commit improve to support various TFmini models and basic autosetup for TFMini-plus

Describe your solution

A new "tfmini command -c XXXXXX" is introduced to help device configuration
directly from the flight controler. TFMini and TFMini-plus are both supported.

The parser has been improved to support receiving confirmation from the device
after a command has been written.

The parameter SENS_EN_TFMINI is implemented again to let user define the TFMini
model that will be used. This helps to validate commands and validate command's answers.

SENS_EN_TFMINI is also required because TFMini models don't offer the same capabilities
for FOV and MIN-DISTANCE.

The parser has been improved to provide signal quality for TFMini-plus.
The same should be done in the future for TFMini model.

The driver has been improved to autoconfigure the mode AND enable the output for TFMini-plus upon starting.
The same should be done in the future for TFMini model.

Describe possible alternatives

The TFMini-plus has signal strength between 100 and 0xFFFF for valid values. However, from testing, values are usually between 100 and 20000 instead. The scaling may not be perfect, but I have decided to measure signal_quality (in percentage) as 100strength/20000 instead of 100strength/0xFFFF with maximum strength being 20000 instead of 0xFFFFF.

Test data / coverage

I have tested the patch on a drone with a flight controler running latest master branch, but without flying. I have no drone ready to fly. Most of the patch concerns configuration, so I don't feel this is critical. Of course, I hope someone can fly with the patch and report

I have implemented as much as I need about the TFMini-plus model, but I have decided to not write the autosetup for the TFMini model because I don't have the device. However, I could write it, if someone is willing to test it.

Here are some example of command lines that can be used to configure the TFmini-plus device using nsh:

Retreive the version on a TFMINI-Plus: (0x5A 0x04 0x01 0x5F)
$ tfmini command -c 5A04015F

Configure TFMINI-Plus in Standard 9 bytes (cm) mode: (0x5A 0x05 0x05 0x01 0x65)
$ tfmini command -c 5A05050165

Configure TFMINI-Plus Frame Rate: (0x5A 0x06 0x03 0xLL 0xHH 0xSU) - Example for 100Hz
$ tfmini command -c 5A06036400C7

Apply and Save Settings for TFMINI-Plus: (0x5A 0x04 0x11 0x6F)
$ tfmini command -c 5A04116F

[dagar]

I have implemented as much as I need about the TFMini-plus model, but I have decided to not write the autosetup for the TFMini model because I don't have the device. However, I could write it, if someone is willing to test it.

I can arrange tfmini testing.

[dagar]

Instead of passing through raw commands did you consider making them a bit higher level? For example requesting the version, configuring the frame rate, etc.

[amoizard]

Tks @dagar for all your comments. I have pushed 4 commits to answer your reports.

• I have added comments to reference the documentation of TFmini and TFmini-plus models.
• I have added comments to explain the command/answers for both TFmini and TFmini-plus models.
• I have added an enum TFMINI_MODEL for code readability
• I have used PX4_INFO instead of printf where you asked. (it's not clear from other drivers as sometimes printf is used, and other times PX4_INFO is used...)

Instead of passing through raw commands did you consider making them a bit higher level? For example requesting the version, configuring the frame rate, etc.

I had the feeling that I should try to minimize the amount of code written, to avoid increasing the final binary size. If a higher level code is provided, it would means translating around 10 commands per devices, same amount of answers, plus the variant of each commands (on/off, or frequency, etc..)

If you feel this is required (at least for interesting commands), it may be better to bring this in a separate pull request?

[dagar]

I have used PX4_INFO instead of printf where you asked. (it's not clear from other drivers as sometimes printf is used, and other times PX4_INFO is used...)

Thanks, there's still a bit too much inconsistency at the moment. Nearly all the printfs should be removed.

If you feel this is required (at least for interesting commands), it may be better to bring this in a separate pull request?

Certainly not required, I was mainly trying to gauge if there's a subset of commands worth implementing directly to actually make the functionality accessible.

[mcsauder]

The PR checks out well with a pixhawk 4 and tfmini plus.

I'll make a few comments in the code but it looks to be in good shape from my perspective! Nice work!

[mcsauder]

If there are a limited number of commands available, it might be easier for the user to abstract the hex values here by giving them variables/names.

[amoizard]

If there are a limited number of commands available, it might be easier for the user to abstract the hex values here by giving them variables/names.

The commands are not limited to any subset. They will be validated for TFmini and TFmini-plus (header and lenght only) and will be rejected if model is not defined.

I will take into account your comments and will push more changes.
Thanks a lot @mcsauder !

[amoizard]

I have implemented as much as I need about the TFMini-plus model, but I have decided to not write the autosetup for the TFMini model because I don't have the device. However, I could write it, if someone is willing to test it.

I can arrange tfmini testing.

I have commited autosetup for TFmini model. As I said, I don't have the device, so this is very blind code...

In order to verify, you need at least to run the command "tfmini status" under nsh with a TFmini model being connected and with SENS_EN_TFMINI being set to 1.

This should display, among other lines:
"TFMINI is configured - version=unknown"

If anything went wrong, please let me know!

[mcsauder]

Hi @amoizard ,

It looks like things are functioning properly.

One detail we could attend to, it appears the driver is returning tfmini::usage() at every start. It might make the driver main() function cleaner if we eliminate the else if chain and just use if statements. (The terraranger and ulanding drivers illustrate that as an example.)

[mcsauder]

@amoizard and @dagar , just an FYI, the tfmini driver is starting and continuing to run even if no tfmini is connected at boot in this PR and in current master. If a device is connected after boot, it recognizes the device and continues to run properly in both branches as well.

[amoizard]

@mcsauder / thanks for testing, but it's not working...

From your snapshot above, the "autosetup" for TFmini doesn't work. It is written "TFmini is being configured" instead of "TFmini is configured". The "state" is still the first one, which means even the first "command" (enter setup) was not accepted.

I advise you to try this command first:

tfmini command -c 4257020000000102

The result must be: (20ms can be different)

command confirmed [20ms] [4257020100000102]

I'm sorry I can't debug this myself. I have looked for a typo or some obvious mistake, but couldn't locate the issue.

[hamishwillee]

FYI, when this goes in, we should update this page: http://docs.px4.io/master/en/sensor/tfmini.html

[amoizard]

@mcsauder , @dagar

I have finally purchased a TFMINI in order to test myself.
With minor change, I was able to fix the autosetup and validate usage of the command with TFMINI model.

As you can see in the snapshot, the "tfmini status" is showing the expected line saying:

INFO [tfmini] TFMINI is configured - version=unknown

Also, setup via the command line also works. The use-case below is showing the sequence:

• enter setup mode
• configure standard mode
• configure unit in cm
• save setting and leave setup mode

[amoizard]

It seems the build failure was related to a timeout which is not related to my patch... Any way to restart the checks?

[mcsauder]

@amoizard , try merging upstream/rebasing and see if we get the same CI failure again.

[mcsauder]

This looks good to me. I'd like to follow up with another PR after this one goes it to do a bit of refactoring and move some of the new method calls out of collect() into init(), but this is a good first step.

Nice work @amoizard !

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[LorenzMeier]

Would you mind rebasing and re-testing this? My apologies that this wasn't followed up on.

[amoizard]

Would you mind rebasing and re-testing this? My apologies that this wasn't followed up on.

I'm sorry: I'm not any more involved on the project and I don't have my px4 any more. I do hope @mcsauder will be able to replace me on this patch?

[mcsauder]

Would you mind rebasing and re-testing this? My apologies that this wasn't followed up on.

I'm sorry: I'm not any more involved on the project and I don't have my px4 any more. I do hope @mcsauder will be able to replace me on this patch?

Sure thing. I'll take care of it! Thanks for your work!

[mcsauder]

Don't merge this yet, I still need to complete functional testing to make sure nothing has become broken in the time since this PR was submitted... I was actually shocked that the CI tests all passed. :)

[mcsauder]

I've rebased this PR and performed a few quick spot checks on the bench. This PR is good to go from what I can observe.

See also PR #17645.