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

AP_RangeFinder: add support for RDS02UF radar driver on seria #23056

Merged
merged 7 commits into from
Apr 1, 2024

Conversation

zebulon-86
Copy link
Contributor

Summary

Resubmit pull request,relevant : #22385
Adding new features
AP_RangeFinder:add support for RDS02UF radar driver on serial。

Documentation

Product URL:Microbrain Intelligent
Related Manuals:【Benewake(北醒)】毫米波雷达在无人机上避障资料汇总
Here are the dumped Manuals:RDS02U_Uart_Manual
For PDF:RDS02U

Change

Some tweaks to the code related to #22385

libraries/AP_RangeFinder/AP_RangeFinder_RDS02UF.h

#if RDS02_USE_CRC
    const uint8_t crc8_table[256] = {
    0x93,0x98,0xE4,0x46,0xEB,0xBA,0x04,0x4C,

The CRC table takes up too much flash space,so added the define of whether to enable CRC, which is not enabled by default, and only use the frame header and frame tail to judge the integrity of the frame. It is not a big problem not to do CRC when the serial communication is stable. Users can also choose to enable CRC according to the hardware situation.

@zebulon-86 zebulon-86 force-pushed the driver_rangefinder branch 2 times, most recently from e2ae54d to 85dc15b Compare March 3, 2023 09:54
@magicrub
Copy link
Contributor

magicrub commented Mar 3, 2023

Why not use existing crc functions?

@zebulon-86
Copy link
Contributor Author

Why not use existing crc functions?

Because the crc8_table used by this sensor is a fixed value defined by the manufacturer, it cannot be calculated by the formula

@zebulon-86 zebulon-86 requested a review from muramura March 4, 2023 03:01
@zebulon-86 zebulon-86 changed the title Driver rangefinder AP_RangeFinder: add support for RDS02UF radar driver on seria Mar 6, 2023
@zebulon-86 zebulon-86 force-pushed the driver_rangefinder branch 2 times, most recently from f5db60a to 6b403e8 Compare March 16, 2023 11:27
@rmackay9
Copy link
Contributor

rmackay9 commented May 7, 2023

Hi @zebulon-86,

It looks like this is failing CI because of a problem with a definition.

[ 469/1015] Compiling libraries/AP_RangeFinder/AP_RangeFinder_PulsedLightLRF.cpp
../../libraries/AP_RangeFinder/AP_RangeFinder_RDS02UF.cpp:155:13: error: "RDS02_USE_CRC" is not defined, evaluates to 0 [-Werror=undef]
155 | #if RDS02_USE_CRC
| ^~~~~~~~~~~~~

@zebulon-86 zebulon-86 force-pushed the driver_rangefinder branch 3 times, most recently from b18d741 to 015fee3 Compare May 8, 2023 13:45
Copy link
Collaborator

@Hwurzburg Hwurzburg left a comment

Choose a reason for hiding this comment

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

this has no metadata update to the rf type param

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label May 8, 2023
@zebulon-86
Copy link
Contributor Author

zebulon-86 commented May 8, 2023

Hi @zebulon-86,

It looks like this is failing CI because of a problem with a definition.

[ 469/1015] Compiling libraries/AP_RangeFinder/AP_RangeFinder_PulsedLightLRF.cpp ../../libraries/AP_RangeFinder/AP_RangeFinder_RDS02UF.cpp:155:13: error: "RDS02_USE_CRC" is not defined, evaluates to 0 [-Werror=undef] 155 | #if RDS02_USE_CRC | ^~~~~~~~~~~~~

Hi @rmackay9 ,
I added in build_options.py

Feature('Rangefinder', 'RANGEFINDER_RDS02UF', 'AP_RANGEFINDER_RDS02UF_ENABLED', "Enable Rangefinder - RDS02UF", 0, "RANGEFINDER"), # NOQA: E501

Added #if AP_RANGEFINDER_RDS02UF_ENABLED in AP_RangeFinder_RDS02UF.cpp, now All checks have passed

@zebulon-86
Copy link
Contributor Author

this has no metadata update to the rf type param

@Hwurzburg
Sorry, I don't quite understand which one this refers to specifically. Is it the following definition?

MAV_DISTANCE_SENSOR _get_mav_distance_sensor_type() const override {
    return MAV_DISTANCE_SENSOR_RADAR;
}

@Hwurzburg
Copy link
Collaborator

Hwurzburg commented May 8, 2023

add metadata in

@rishabsingh3003
Copy link
Contributor

From Dev Call discussion - Redo the implementation with structure

@tridge
Copy link
Contributor

tridge commented Mar 4, 2024

@peterbarker will review parser and merge

@rishabsingh3003
Copy link
Contributor

Added "memchr"

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Please test this with a little bit of corruption, make sure it still generally works (AP_SIM_SERIALDEVICE_CORRUPTION_ENABLED). Run under valgrind when doing this.

@rishabsingh3003
Copy link
Contributor

rebased, and fixed the pending comments

@rishabsingh3003
Copy link
Contributor

Also tested with, AP_SIM_SERIALDEVICE_CORRUPTION_ENABLED and valgrind. Works well.
Tested with real sensor connected to SITL as well.

Comment on lines +100 to +101
// reset buffer
body_length = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// reset buffer
body_length = 0;
// consume packet
move_header_in_buffer(sizeof(u.packet));

The current fixed size makes this NFC, but if we wanted to be able to read multiple packets at once this would be required....

move_header_in_buffer(0);

// header byte 1 is correct.
if (body_length < ARRAY_SIZE(u.parse_buffer)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (body_length < ARRAY_SIZE(u.parse_buffer)) {
if (body_length < sizeof(u.packet)) {

... this is, after all, exactly what you're interested in.

The parse buffer can be larger than the packet, allowing for multiple packets to be read in at once (and different packets to be parsed out of the same buffer, of course)


union RDS02UF_Union {
uint8_t parse_buffer[21];
struct RDS02UFPacket packet;
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically we really should mark this as PACKED. Since all members are 8-byte quantities it doesn't really matter.

@peterbarker peterbarker merged commit b22e4fa into ArduPilot:master Apr 1, 2024
91 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks all!

@Hwurzburg
Copy link
Collaborator

@zebulon-86 I can find no vendor offering this device and Benewake does not list on its web page.....I cant see any reason to have a wiki entry for this device since its not available

@rishabsingh3003
Copy link
Contributor

@Hwurzburg I believe Benewake plans to sell these. We have a chat with them where we could ask

@Hwurzburg
Copy link
Collaborator

@rishabsingh3003 I see no chat in Skype or Discord for them.....if someone can give me a link as to where to obtain this device, I will create a wiki page and entry then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants