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

Added gps and mag UAVCAN support to SITL #8509

Closed
wants to merge 8 commits into from
Closed

Conversation

d-v
Copy link
Contributor

@d-v d-v commented May 27, 2018

Added gps and mag over UAVCAN simulation to SITL. Tested on bare metal ubuntu 16.04 LTS.

Added another launch arg to sim_vehicle.py: --uavcan . I tested on vcan0 under ArduPilot. When UAVCAN is enabled, sim_vehicle.py uses -uavcan.parm. I created copter-uavcan.parm but haven't done so for other vehicles. copter-uavcan.parm is essentially the same as copter.parm, just some uavcan-specific params used.

sim_vehicle.py --console --uavcan vcan0 -j4 is what I used to run SITL.

Many thanks to everyone who answered my questions on Gitter and Skype, I am really grateful for the help. Looking forward to hearing everyone's feedback!

Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

excellent !
there will be plenties things to correct but that is good start !
The first step will be to splip you commit at least by directory.
After that I don't think that UAVCAN should be an hal subtype.
We don't want exception active too.
I will make more comments later.

About the usage. It allow SITL backend to access can bus with SLCAN right ? (i think it miss some check on waf for that, but it could be done later). As we already got support for CAN device in normal code, why update them in SITL backend ?

@d-v d-v force-pushed the master branch 2 times, most recently from 8a1492c to 9bb1cd9 Compare May 27, 2018 18:36
@khancyr
Copy link
Contributor

khancyr commented May 28, 2018

@d-v To help us on reviewing, start by separate your commit by functionnality !
http://ardupilot.org/dev/docs/git-interactive-rebase.html and the script git-subsystems-split in Tools/gittools will help you doing that !

Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

Ok I put some comments to address in order to sort things up !
From a general point of view, uavcan implementation should replace normal sensors !

@@ -34,29 +34,29 @@ const AP_Param::Info Copter::var_info[] = {
// @Description: This value is incremented when changes are made to the eeprom format
// @User: Advanced
// @ReadOnly: True
GSCALAR(format_version, "SYSID_SW_MREV", 0),
GSCALAR(format_version, "SYSID_SW_MREV", 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

the whitespaces change on parameter won't pass. So better revers those changes

@@ -31,7 +31,7 @@ class Board:
abstract = True

def __init__(self):
self.with_uavcan = False
self.with_uavcan = True
Copy link
Contributor

Choose a reason for hiding this comment

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

this is for all board, and we don't want that. Just override it on SITL config

@@ -110,7 +110,7 @@ def configure_env(self, cfg, env):

'-fdata-sections',
'-ffunction-sections',
'-fno-exceptions',
'-fexceptions',
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't want to use exception. revert that

Copy link
Contributor

Choose a reason for hiding this comment

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

look at how it is done on linux :
if self.with_uavcan:
cfg.define('UAVCAN_EXCEPTIONS', 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't work :(
https://gist.github.com/d-v/508d4c66f548f3301f0384dd627828bf
Even with cfg.define(UAVCAN_EXCEPTIONS, 0)

if self.with_uavcan:
env.DEFINES.update(
CONFIG_HAL_BOARD = 'HAL_BOARD_SITL',
CONFIG_HAL_BOARD_SUBTYPE = 'HAL_WITH_UAVCAN',
Copy link
Contributor

Choose a reason for hiding this comment

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

this is wrong

if self.with_uavcan:
env.INCLUDES += [
cfg.srcnode.find_dir('modules/uavcan/libuavcan_drivers/linux/include/').abspath(),
cfg.srcnode.find_dir('modules/uavcan/libuavcan/include/').abspath()
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is already define in board config

@@ -54,6 +54,7 @@ SIM_MAG_RND 0
SIM_GPS_GLITCH_X 0
SIM_GPS_GLITCH_Y 0
SIM_GPS_GLITCH_Z 0
GPS_TYPE 1.000000
Copy link
Contributor

Choose a reason for hiding this comment

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

this is already the case no?

@@ -566,6 +566,9 @@ def start_vehicle(binary, autotest, opts, stuff, loc):
path = None
if "default_params_filename" in stuff:
paths = stuff["default_params_filename"]
if opts.uavcan:
suffix = paths.find(".")
paths = paths[:suffix]+"-uavcan"+paths[suffix:]
Copy link
Contributor

Choose a reason for hiding this comment

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

paths = paths[:suffix] + "-uavcan" + paths[suffix:]

Copy link
Contributor

Choose a reason for hiding this comment

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

see my previos comment, it will be simplier to load only the config for uavacan instead of all params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@khancyr, I'm sorry I don't understand what you mean :(

@@ -80,15 +101,26 @@ void SITL_State::_sitl_setup(const char *home_str)
_barometer = (AP_Baro *)AP_Param::find_object("GND_");
_ins = (AP_InertialSensor *)AP_Param::find_object("INS_");
_compass = (Compass *)AP_Param::find_object("COMPASS_");
#if HAL_WITH_UAVCAN
#if CONFIG_HAL_BOARD == HAL_BOARD_SITL
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed as you are already on SITL namespace

@@ -261,12 +269,26 @@ def configure_env(self, cfg, env):
'SITL',
]

env.GIT_SUBMODULES += [
'uavcan',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should check for SLCAN or whatever is the name for the linux implementation of CAN bus !

bool configured(void) const
{
#if HAL_WITH_UAVCAN
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't look good

@d-v
Copy link
Contributor Author

d-v commented May 28, 2018

@khancyr, thank you for all of the feedback it was really helpful especially on my first real PR. I'll incorporate these changes and update the PR.

@d-v d-v force-pushed the master branch 5 times, most recently from 2edad43 to d682502 Compare June 3, 2018 05:30
@magicrub
Copy link
Contributor

magicrub commented Jun 3, 2018

Please take a look at the guidelines
http://ardupilot.org/dev/docs/submitting-patches-back-to-master.html

commit comments should start with the directory name.

Also, you now have files in this PR, so something went crazy again. I'm sure a rebase would fix it

@d-v
Copy link
Contributor Author

d-v commented Jun 4, 2018

@magicrub, thanks for the feedback -- sorry that this has been such a messy PR! I'm still working on one residual issue that I'm fairly certain will allow all the tests to pass. Once that's done I'll be able to clean up and break this up into a bunch of commits that are properly styled. Sorry about the confusion!

@d-v d-v force-pushed the master branch 4 times, most recently from 68b0171 to 8e320de Compare June 9, 2018 14:35
@bugobliterator bugobliterator self-requested a review July 12, 2018 09:14
@auturgy auturgy mentioned this pull request Jul 30, 2018
Dimitri Vasilkov added 3 commits August 3, 2018 12:04
…in to spinOnce in AP_UAVCAN.cpp because spin was never exiting on Ubuntu
@d-v d-v force-pushed the master branch 2 times, most recently from d10a043 to 6fe953d Compare August 6, 2018 18:06
@davidbuzz
Copy link
Collaborator

i liked this old PR enough to try re-working it based on current ArduPilot. My rework of the code doesn't seem to compile right now when I add "--uavcan vcan0" to sim_vehicle.py , as a result of libuavcan incompatibilities with the linux can driver etc. anyway.... i'm wondering if the original author of this PR, or anyone else, is interested enough to help me figure out where my revised branch needs work... https://github.com/davidbuzz/ardupilot/tree/uavcan-sitl-buzz-wip ?

@khancyr
Copy link
Contributor

khancyr commented Dec 29, 2020

We got a proper implementation using AP_Periph for those now. I am closing this. Thanks anyway

@khancyr khancyr closed this Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants