-
Notifications
You must be signed in to change notification settings - Fork 17
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
RSDK-4106: Restructure lidar name and data rate to use map #213
Conversation
cartofacade/capi.go
Outdated
@@ -99,7 +99,7 @@ const ( | |||
|
|||
// CartoConfig contains config values from app | |||
type CartoConfig struct { | |||
Sensors []string | |||
Camera map[string]string |
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.
Camera map[string]string | |
CameraName string |
cartofacade/capi.go
Outdated
@@ -179,7 +179,7 @@ func NewCarto(cfg CartoConfig, acfg CartoAlgoConfig, vcl CartoLibInterface) (Car | |||
return Carto{}, err | |||
} | |||
|
|||
status = C.free_bstring_array(vcc.sensors, C.size_t(len(cfg.Sensors))) | |||
status = C.free_bstring_array(vcc.sensors, C.size_t(1)) |
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.
lets change the C API to take a lidar & optionally an imu, rather than having a sensor list
cartofacade/testhelpers.go
Outdated
@@ -12,7 +12,7 @@ func GetTestConfig(sensor string) (CartoConfig, string, error) { | |||
} | |||
|
|||
return CartoConfig{ | |||
Sensors: []string{sensor, "imu"}, | |||
Camera: map[string]string{"name": "test_lidar", "data_freq_hz": "20"}, |
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.
cartographer doesn't need to know the data_frequency_hz does it?
config/config.go
Outdated
if !ok { | ||
return nil, utils.NewConfigValidationError(path, errCameraMustHaveName) | ||
} | ||
dataFreqHz, ok := config.Camera["data_freq_hz"] |
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.
The scope doc says this is data_frequency_hz
config/config.go
Outdated
if ok { | ||
dataFreqHz, err := strconv.Atoi(dataFreqHz) | ||
if err != nil { | ||
return nil, errors.New("data_freq_hz must only contain digits") |
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 should specify we are talking about camera[data_frequency_hz]
sensors/sensors.go
Outdated
var err error | ||
dataFreqHz, err = strconv.Atoi(dataFreqHzIn) | ||
if err != nil { | ||
logger.Debug("data_freq_hz must only contain digits, setting to default value of %d", defaultDataFreqHz) |
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 should be an error
} | ||
|
||
newLidar, err := camera.FromDependencies(deps, name) | ||
if err != nil { | ||
return Lidar{}, errors.Wrapf(err, "error getting lidar camera %v for slam service", name) | ||
} | ||
// If there is a camera provided in the 'camera' field, we enforce that it supports PCD. | ||
properties, err := newLidar.Properties(ctx) |
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 do we need to do 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.
because we are now calling it a camera, so users may feel the need to add any kind of camera that does not actually support PCDs
update_latest_global_pose = true; | ||
tmp_global_pose = map_builder.GetGlobalPose(local_poses.back()); | ||
} | ||
tmp_global_pose = map_builder.GetGlobalPose(); |
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 changing?
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.
oh I thought I changed that back - sorry will fix
@@ -31,11 +31,6 @@ MapBuilder::~MapBuilder() { | |||
} | |||
} | |||
|
|||
std::vector<::cartographer::transform::Rigid3d> |
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 changing?
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.
No idea I did not touch that... will fix
// GetGlobalPose returns the local pose based on the provided a local pose. | ||
cartographer::transform::Rigid3d GetGlobalPose( |
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 changing?
@@ -396,12 +394,12 @@ type CartographerService struct { | |||
|
|||
configParams map[string]string | |||
dataDirectory string | |||
sensors []string | |||
camera map[string]string |
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 recommend keeping the cameraName & the data rate msec separate.
Also you can probably migrate all users of primarySensorName to be the cameraName
Edit lidar naming and config
Build data frequency into lidar map rather than separate variable