-
Notifications
You must be signed in to change notification settings - Fork 191
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
Femtomes PX4 gps device driver #49
Conversation
Thanks for the contribution. |
This is for their Femtomes FB672 and FB680 series receiver boards. |
Hi @Femtomes Then we link that from the parent page: https://docs.px4.io/master/en/gps_compass/rtk_gps.html |
Hi @hamishwillee |
src/femtomes.h
Outdated
#endif | ||
|
||
|
||
GPSDriverFemto::GPSDriverFemto(GPSCallbackPtr callback, void *callback_user, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code in the header file? It is duplicated from the .cpp file.
Also the class declaration for GPSDriverFemto
is missing, so this will not compile, and the pull request is incomplete in this state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so sorry i can't understand about your answer.
1、 the 68 line will complete the init work when there have GPSDriverFemto object be create
2、this class declaration have done in the femtomes.h, you see the 68 line is in femtomes.cpp
3、I have do test about this code, we have log and video. so it sure to can be compiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at femtones.h and femtones.cpp in this PR: they both contain the same content, so it cannot work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry about have not notice the commit file of femtomes.h same to the femtomes.cpp. Later we will fix this problem. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have recommit the femtomes.h, please check this again,thank you.
@Femtomes Re #49 (comment) - sounds good. There is information on working with the documentation toolchain here: https://dev.px4.io/master/en/contribute/docs.html. Generally my advice is to copy the patterns of existing similar material. After you've created a PR I may have more to add :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked through it in detail and added some remarks and suggestions.
src/femtomes.h
Outdated
OutputMode _output_mode{ OutputMode::GPS }; | ||
bool _correction_output_activated{false}; | ||
bool _configure_done{false}; | ||
float _heading_offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be substracted from the heading. Please check in ashtech.cpp
how it's done.
Is heading supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i have not understand about this .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this have changed in the code, thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. You need to consider the wrapping as well: https://github.com/PX4/GpsDrivers/blob/master/src/ashtech.cpp#L303.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so sorry, i have change as your suggest this time。
src/femtomes.cpp
Outdated
} | ||
} | ||
#endif | ||
if (writeAckedCommandFemto("LOG UAVGPSB 0.2\r\n", "<LOG OK",FEMO_RESPONSE_TIMEOUT) == 0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that 5Hz? Can it go higher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can go higher,there need 20HZ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you can use 20Hz.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this have changed 5HZ to 20HZ in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't sure how hz px4 need, our board can support max 20HZ.
src/femtomes.cpp
Outdated
} | ||
} | ||
#endif | ||
if (writeAckedCommandFemto("LOG UAVGPSB 0.2\r\n", "<LOG OK",FEMO_RESPONSE_TIMEOUT) == 0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can go higher,there need 20HZ?
src/femtomes.h
Outdated
#define FEMTO_PREAMBLE3 0x12 | ||
|
||
|
||
class GPSDriverFemto : public GPSBaseStationSupport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, in this time, i change the class GPSDriverFemto : public GPSHelper,
src/femtomes.h
Outdated
OutputMode _output_mode{ OutputMode::GPS }; | ||
bool _correction_output_activated{false}; | ||
bool _configure_done{false}; | ||
float _heading_offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i have not understand about this .
Hi @hamishwillee thank you |
@Femtomes I have requested access to the first link, the second link is broken. Do you want to try creating a page like the one above? The information on how is in http://dev.px4.io/master/en/contribute/docs.html#adding-new-pages-and-images---big-changes It is easy enough - you copy the format of the one I linked to and then modify it to fill in your information. |
Hi @hamishwillee Hi @bkueng |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @Femtomes, looks much better already.
Can you provide me access to the logs?
src/femtomes.h
Outdated
femto_msg_t _femto_msg; | ||
FemtoDecodeState _decode_state{FemtoDecodeState::pream_ble1}; | ||
|
||
bool _got_pashr_pos_message{false}; /**< If we got a PASHR,POS message, we will ignore GGA messages */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_got_pashr_pos_message
is still unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks,i have delete all not used variables and functions
src/femtomes.h
Outdated
#define FEMTO_PREAMBLE3 0x12 | ||
|
||
|
||
class GPSDriverFemto : public GPSBaseStationSupport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Femtomes did you see my comment here?
src/femtomes.cpp
Outdated
baudrate = desired_baudrate; | ||
const char baud_config[] = "com%c 115200\r\n"; // configure baudrate to 115200 | ||
char baud_config_str[sizeof(baud_config)]; | ||
int len = snprintf(baud_config_str, sizeof(baud_config_str), baud_config, _port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_port
is just a space (' '), so this can be simplified. Did you intend to make this configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, delete this already.
@Femtomes Friendly ping: There are open review comments / questions. |
@LorenzMeier Thanks, I‘m asking them the new progress. |
Log link: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the log, looks really good.
Do you have one with faster movements as well?
src/femtomes.cpp
Outdated
uint8_t buf[GPS_READ_BUFFER_SIZE]; | ||
gps_abstime time_started = gps_absolute_time(); | ||
while (time_started + timeout * 1000 * 2 > gps_absolute_time()) { | ||
int ret = read(buf, sizeof(buf), 1000); /**< wait 1000us */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a factor of * 2
above?
The timeout is in ms, and should be used from the argument:
int ret = read(buf, sizeof(buf), 1000); /**< wait 1000us */ | |
int ret = read(buf, sizeof(buf), timeout); | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks,this is my fault,not need * 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkueng the log of faster movements need some time.
src/femtomes.h
Outdated
class RTCMParsing; | ||
|
||
/* ms, timeout for waiting for a response*/ | ||
#define FEMO_RESPONSE_TIMEOUT 200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo (below as well):
#define FEMO_RESPONSE_TIMEOUT 200 | |
#define FEMTO_RESPONSE_TIMEOUT 200 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks,fixed this
Femtomes is one product of gps receiver, it support the gps location, because many px4 customers use, so we develop the driver to be easy for further development and use.