Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: add support for TeraRanger Evo #7962

Merged
merged 4 commits into from
Sep 15, 2017
Merged

drivers: add support for TeraRanger Evo #7962

merged 4 commits into from
Sep 15, 2017

Conversation

msadowski
Copy link
Contributor

Terabee is about to release a new range finder sensor. This commit adds a driver for it.

Any feedback is welcome and highly appreciated.

This commit adds i2c support for TeraRanger Evo sensor
by Terabee

Signed-off-by: Mateusz Sadowski <msadowski90@gmail.com>
@@ -0,0 +1,43 @@
############################################################################
#
# Copyright (c) 2015 PX4 Development Team. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update the year please

@@ -1088,6 +1088,16 @@ PARAM_DEFINE_INT32(SENS_EN_SF0X, 0);
PARAM_DEFINE_INT32(SENS_EN_MB12XX, 0);

/**
* TeraRanger Evo (trevo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a TeraRanger sensor, I think it would be better to have a parameter SENS_EN_TR and add the option to set to 0, if none, 1 if TeraRanger One, 2 if TeraRanger Evo.

@@ -0,0 +1,938 @@
/****************************************************************************
*
* Copyright (c) 2013-2015 PX4 Development Team. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

year

/* Configuration Constants */
#define TREVO_BUS PX4_I2C_BUS_EXPANSION
#define TREVO_BASEADDR 0x31 /* 7-bit address */
#define TREVO_DEVICE_PATH "/dev/trevo"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation please


/* TREVO Registers addresses */

#define TREVO_MEASURE_REG 0x00 /* Measure range register */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation please

virtual int init();

virtual ssize_t read(struct file *filp, char *buffer, size_t buflen);
virtual int ioctl(struct file *filp, int cmd, unsigned long arg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation please

ringbuffer::RingBuffer *_reports;
bool _sensor_ok;
uint8_t _valid;
int _measure_ticks;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation please

* @param address The I2C bus address to probe.
* @return True if the device is present.
*/
int probe_address(uint8_t address);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation please

* Perform a poll cycle; collect from the previous measurement
* and start a new one.
*/
void cycle();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix indentation please

@LorenzMeier
Copy link
Member

Awesome! Good to go if you could address the comments from @TSC21

@TSC21
Copy link
Member

TSC21 commented Sep 13, 2017

@msadowski CI is failing on format check so you should address that by fixing style.

@ndepal
Copy link
Contributor

ndepal commented Sep 13, 2017

The only difference between the TR One and TR Evo drivers are the base address and the min/max distances, right?

What do you think about having only one driver that works for both? Either the user chooses which device they want to use it with via parameter (as per @TSC21's suggestion) or we auto-detect which sensor is connected by probing the I2C base addresses.

@TSC21
Copy link
Member

TSC21 commented Sep 13, 2017

auto-detect which sensor is connected by probing the I2C base addresses.

@ndepal that seems an awesome suggestion. @msadowski can you address this instead IOT simplify the driver implementation?

@ndepal
Copy link
Contributor

ndepal commented Sep 13, 2017

When I was testing out the Evo a while back I actually did that. I never got around to testing that the TR One still works with that change and therefore didn't end up making a PR. You can see the implementation here: ndepal@85483c2

One problem might be if there are other devices with the same I2C address. E.g. an Evo and another device with the TR One address. The teraranger driver might then end up thinking that there is a TR One. One way to work around that would be to have the parameter be 0 = off, 1 = autodetect, 2 = TR One, 3 = TR Evo, but I'm not sure if it's worth doing that, it might not be a real issue.

@TSC21
Copy link
Member

TSC21 commented Sep 13, 2017

0 = off, 1 = autodetect, 2 = TR One, 3 = TR Evo

👍

@msadowski
Copy link
Contributor Author

@ndepal thanks for your contribution, I can test it tomorrow and give you feedback on it.

We were actually discussing within the team about having one or more drivers. One driver is obviously cleaner but what I'm worrying about are cases when someone would like to use both TeraRanger One and Evo (for example one of them pointing forward).

@TSC21 If you don't think it's likely to be an issue for anybody I'm more than happy to test @ndepal implementation and if needed fix it and apply it.

@TSC21
Copy link
Member

TSC21 commented Sep 13, 2017

I'm more than happy that you do it. Thanks

This commit changes old trone driver into a generic
TeraRanger driver that supports both TeraRanger One
and TeraRanger Evo.

As a part of the change a new parameter was created
SENS_EN_TRANGER that allows to specify the following
modes of operation:

0 - sensors disabled
1 - autodetect sensors
2 - use TeraRanger One rangefinder
3 - use TeraRanger Evo rangefinder

Signed-off-by: Mateusz Sadowski <msadowski90@gmail.com>
Signed-off-by: Mateusz Sadowski <msadowski90@gmail.com>
@msadowski
Copy link
Contributor Author

I updated the driver to take the 4 parameters (disabled, automatic, One, Evo), fixed up the copyright dates and tested it with both TeraRanger One and Evo. Everything seems to work now. Please let me know if there is anything you would like us to refactor further.

@ndepal
Copy link
Contributor

ndepal commented Sep 14, 2017

Great! Looks good to me.

@TSC21
Copy link
Member

TSC21 commented Sep 14, 2017

@msadowski awesome work! BTW any plans to add a POSIX driver to the TeraRanger products?

@TSC21 TSC21 closed this Sep 14, 2017
@TSC21 TSC21 reopened this Sep 14, 2017
@msadowski
Copy link
Contributor Author

@TSC21 POSIX driver would be truly neat, I'll ask around to see what others think. I will make sure to record it in our backlog as well.

@TSC21 TSC21 closed this Sep 15, 2017
@TSC21 TSC21 reopened this Sep 15, 2017
@msadowski
Copy link
Contributor Author

@TSC21: quick follow up: could you please tell me what benefits do you see as a user to have the POSIX driver?

@TSC21
Copy link
Member

TSC21 commented Sep 15, 2017

You have different platforms that can there run it - RPi+Navio, Bebop, OcPoC... Or any linux system running PX4 POSIX.

@TSC21 TSC21 merged commit 8ba6c20 into PX4:master Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants