Skip to content

Commit

Permalink
RSDK-4610 Create New Config Flag (#276)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jeremyrhyde
jeremyrhyde authored Sep 28, 2023
1 parent 1c4f6ea commit 3e26b32
Show file tree
Hide file tree
Showing 6 changed files with 97 additions and 96 deletions.
24 changes: 13 additions & 11 deletions config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,21 @@ func newError(configError string) error {

// Config describes how to configure the SLAM service.
type Config struct {
Camera map[string]string `json:"camera"`
MovementSensor map[string]string `json:"movement_sensor"`
ConfigParams map[string]string `json:"config_params"`
DataDirectory string `json:"data_dir"`
MapRateSec *int `json:"map_rate_sec"`
IMUIntegrationEnabled bool `json:"imu_integration_enabled"`
Sensors []string `json:"sensors"`
DataRateMsec *int `json:"data_rate_msec"`
Camera map[string]string `json:"camera"`
MovementSensor map[string]string `json:"movement_sensor"`
ConfigParams map[string]string `json:"config_params"`
DataDirectory string `json:"data_dir"`
MapRateSec *int `json:"map_rate_sec"`
Sensors []string `json:"sensors"`
DataRateMsec *int `json:"data_rate_msec"`

CloudStoryEnabled bool `json:"cloud_story_enabled"`
ExistingMap string `json:"existing_map"`
EnableMapping *bool `json:"enable_mapping"`
UseCloudSlam *bool `json:"use_cloud_slam"`
// NewConfigFlag is used to access the config changes from imu and cloud story work while providing backwards compatibility
// To be removed during JIRA Ticket: RSDK-4742 https://viam.atlassian.net/browse/RSDK-4742
NewConfigFlag bool `json:"new_config_flag"`
}

// OptionalConfigParams holds the optional config parameters of SLAM.
Expand All @@ -54,7 +56,7 @@ func (config *Config) Validate(path string) ([]string, error) {
cameraName := ""
imuName := ""
imuExists := false
if config.IMUIntegrationEnabled {
if config.NewConfigFlag {
var ok bool
cameraName, ok = config.Camera["name"]
if !ok {
Expand Down Expand Up @@ -111,7 +113,7 @@ func GetOptionalParameters(config *Config, defaultLidarDataRateMsec, defaultIMUD
) (OptionalConfigParams, error) {
var optionalConfigParams OptionalConfigParams
// feature flag for new config
if config.IMUIntegrationEnabled {
if config.NewConfigFlag {
strCameraDataFreqHz, exists := config.Camera["data_frequency_hz"]
if !exists {
optionalConfigParams.LidarDataRateMsec = defaultLidarDataRateMsec
Expand Down Expand Up @@ -174,7 +176,7 @@ func GetOptionalParameters(config *Config, defaultLidarDataRateMsec, defaultIMUD
optionalConfigParams.EnableMapping = *config.EnableMapping
}

if err := validateModes(optionalConfigParams, config.IMUIntegrationEnabled); err != nil {
if err := validateModes(optionalConfigParams, config.NewConfigFlag); err != nil {
return OptionalConfigParams{}, err
}

Expand Down
77 changes: 38 additions & 39 deletions config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

func testValidateTesthelper(
t *testing.T,
imuIntegrationEnabled bool,
newConfigFlag bool,
cloudStoryEnabled bool,
suffix string,
) {
Expand All @@ -24,35 +24,34 @@ func testValidateTesthelper(
t.Run(fmt.Sprintf("Empty config %s", suffix), func(t *testing.T) {
model := resource.DefaultModelFamily.WithModel("test")
cfgService := resource.Config{Name: "test", API: slam.API, Model: model}
if imuIntegrationEnabled || cloudStoryEnabled {
if newConfigFlag || cloudStoryEnabled {
cfgService.Attributes = make(map[string]interface{})
}

if imuIntegrationEnabled {
cfgService.Attributes["imu_integration_enabled"] = true
if newConfigFlag {
cfgService.Attributes["new_config_flag"] = true
}

if cloudStoryEnabled {
cfgService.Attributes["cloud_story_enabled"] = true
}
_, err := newConfig(cfgService)

if imuIntegrationEnabled {
if newConfigFlag {
test.That(t, err, test.ShouldBeError, newError("error validating \"services.slam.attributes.fake\": \"camera[name]\" is required"))
} else {
test.That(t, err, test.ShouldBeError, newError("error validating \"services.slam.attributes.fake\": \"sensors\" must not be empty"))
}
})

t.Run(fmt.Sprintf("Simplest valid config %s", suffix), func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
_, err := newConfig(cfgService)
test.That(t, err, test.ShouldBeNil)
})

t.Run(fmt.Sprintf("Config without required fields %s", suffix), func(t *testing.T) {
requiredFields := getRequiredFields(imuIntegrationEnabled, cloudStoryEnabled)

requiredFields := getRequiredFields(newConfigFlag, cloudStoryEnabled)
dataDirErr := utils.NewConfigValidationFieldRequiredError(testCfgPath, "data_dir")
sensorErr := utils.NewConfigValidationError(testCfgPath, errSensorsMustNotBeEmpty)
cameraErr := utils.NewConfigValidationError(testCfgPath, errCameraMustHaveName)
Expand All @@ -64,15 +63,15 @@ func testValidateTesthelper(
}
for _, requiredField := range requiredFields {
logger.Debugf("Testing SLAM config without %s\n", requiredField)
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
delete(cfgService.Attributes, requiredField)
_, err := newConfig(cfgService)

test.That(t, err, test.ShouldBeError, expectedErrors[requiredField])
}
// Test for missing config_params attributes
logger.Debug("Testing SLAM config without config_params[mode]")
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
delete(cfgService.Attributes["config_params"].(map[string]string), "mode")
_, err := newConfig(cfgService)
test.That(t, err, test.ShouldBeError, newError(utils.NewConfigValidationFieldRequiredError(testCfgPath, "config_params[mode]").Error()))
Expand All @@ -81,7 +80,7 @@ func testValidateTesthelper(
t.Run(fmt.Sprintf("Config with invalid parameter type %s", suffix), func(t *testing.T) {
key := "data_dir"

cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
cfgService.Attributes[key] = true

_, err := newConfig(cfgService)
Expand All @@ -96,8 +95,8 @@ func testValidateTesthelper(

t.Run(fmt.Sprintf("Config with out of range values %s", suffix), func(t *testing.T) {
var mapRateSecError error
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
if imuIntegrationEnabled {
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
if newConfigFlag {
cfgService.Attributes["camera"] = map[string]string{
"name": "a",
"data_frequency_hz": "-1",
Expand Down Expand Up @@ -130,7 +129,7 @@ func testValidateTesthelper(
})

t.Run(fmt.Sprintf("All parameters e2e %s", suffix), func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
cfgService.Attributes["sensors"] = []string{"a", "b"}
cfgService.Attributes["data_rate_msec"] = 1001
cfgService.Attributes["map_rate_sec"] = 1002
Expand All @@ -150,10 +149,10 @@ func testValidateTesthelper(
})
}

func getRequiredFields(imuIntegrationEnabled, cloudStoryEnabled bool) []string {
func getRequiredFields(newConfigFlag, cloudStoryEnabled bool) []string {
requiredFields := []string{}

if imuIntegrationEnabled {
if newConfigFlag {
requiredFields = append(requiredFields, "camera")
} else {
requiredFields = append(requiredFields, "sensors")
Expand All @@ -166,25 +165,25 @@ func getRequiredFields(imuIntegrationEnabled, cloudStoryEnabled bool) []string {
}

func TestValidate(t *testing.T) {
for _, imuEnabled := range []bool{true, false} {
for _, newConfigFlag := range []bool{true, false} {
for _, cloudStoryEnabled := range []bool{true, false} {
suffix := fmt.Sprintf("with imuIntegrationEnabled = %t & cloudStoryEnabled = %t", imuEnabled, cloudStoryEnabled)
testValidateTesthelper(t, imuEnabled, cloudStoryEnabled, suffix)
suffix := fmt.Sprintf("with newConfigFlag = %t & cloudStoryEnabled = %t", newConfigFlag, cloudStoryEnabled)
testValidateTesthelper(t, newConfigFlag, cloudStoryEnabled, suffix)
}
}
}

// makeCfgService creates the simplest possible config that can pass validation.
func makeCfgService(IMUIntegrationEnabled, cloudStoryEnabled bool) resource.Config {
func makeCfgService(NewConfigFlag, cloudStoryEnabled bool) resource.Config {
model := resource.DefaultModelFamily.WithModel("test")
cfgService := resource.Config{Name: "test", API: slam.API, Model: model}
cfgService.Attributes = map[string]interface{}{
"config_params": map[string]string{"mode": "test mode"},
"data_dir": "path",
}

if IMUIntegrationEnabled {
cfgService.Attributes["imu_integration_enabled"] = true
if NewConfigFlag {
cfgService.Attributes["new_config_flag"] = true
cfgService.Attributes["camera"] = map[string]string{
"name": "a",
}
Expand All @@ -201,14 +200,14 @@ func makeCfgService(IMUIntegrationEnabled, cloudStoryEnabled bool) resource.Conf

func getOptionalParametersTestHelper(
t *testing.T,
imuIntegrationEnabled bool,
newConfigFlag bool,
cloudStoryEnabled bool,
suffix string,
) {
logger := golog.NewTestLogger(t)

t.Run(fmt.Sprintf("Pass default parameters %s", suffix), func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
cfgService.Attributes["sensors"] = []string{"a"}
cfg, err := newConfig(cfgService)
test.That(t, err, test.ShouldBeNil)
Expand All @@ -230,8 +229,8 @@ func getOptionalParametersTestHelper(
})

t.Run(fmt.Sprintf("Return overrides %s", suffix), func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
if imuIntegrationEnabled {
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
if newConfigFlag {
cfgService.Attributes["camera"] = map[string]string{
"name": "testNameCamera",
"data_frequency_hz": "2",
Expand Down Expand Up @@ -259,7 +258,7 @@ func getOptionalParametersTestHelper(
1002,
logger)
test.That(t, err, test.ShouldBeNil)
if imuIntegrationEnabled {
if newConfigFlag {
test.That(t, optionalConfigParams.ImuName, test.ShouldEqual, "testNameSensor")
test.That(t, optionalConfigParams.ImuDataRateMsec, test.ShouldEqual, 500)
test.That(t, optionalConfigParams.LidarDataRateMsec, test.ShouldEqual, 500)
Expand All @@ -277,7 +276,7 @@ func getOptionalParametersTestHelper(
})

t.Run(fmt.Sprintf("Pass invalid existing map %s", suffix), func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
cfgService.Attributes["existing_map"] = "test-file"
cfg, err := newConfig(cfgService)
test.That(t, err, test.ShouldBeNil)
Expand All @@ -298,10 +297,10 @@ func getOptionalParametersTestHelper(
})

t.Run(fmt.Sprintf("config that puts cartographer in offline mode and in localization mode %s", suffix), func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
cfgService.Attributes["existing_map"] = "test-file.pbstream"
cfgService.Attributes["enable_mapping"] = false
if imuIntegrationEnabled {
if newConfigFlag {
cfgService.Attributes["camera"] = map[string]string{
"name": "testcam",
"data_frequency_hz": "0",
Expand All @@ -318,7 +317,7 @@ func getOptionalParametersTestHelper(
1002,
logger)
if cloudStoryEnabled {
if imuIntegrationEnabled {
if newConfigFlag {
test.That(t, err, test.ShouldBeError, errLocalizationInOfflineModeIMU)
} else {
test.That(t, err, test.ShouldBeError, errLocalizationInOfflineMode)
Expand All @@ -331,19 +330,19 @@ func getOptionalParametersTestHelper(
}
})

if imuIntegrationEnabled {
sensorAttributeTestHelper(t, logger, imuIntegrationEnabled, cloudStoryEnabled)
if newConfigFlag {
sensorAttributeTestHelper(t, logger, newConfigFlag, cloudStoryEnabled)
}
}

func sensorAttributeTestHelper(
t *testing.T,
logger *zap.SugaredLogger,
imuIntegrationEnabled bool,
newConfigFlag bool,
cloudStoryEnabled bool,
) {
t.Run("Unit test return error if lidar data frequency is invalid", func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
cfgService.Attributes["camera"] = map[string]string{
"name": "a",
"data_frequency_hz": "b",
Expand All @@ -360,7 +359,7 @@ func sensorAttributeTestHelper(
})

t.Run("Unit test return error if imu data frequency is invalid", func(t *testing.T) {
cfgService := makeCfgService(imuIntegrationEnabled, cloudStoryEnabled)
cfgService := makeCfgService(newConfigFlag, cloudStoryEnabled)
cfgService.Attributes["camera"] = map[string]string{
"name": "a",
"data_frequency_hz": "1",
Expand All @@ -382,10 +381,10 @@ func sensorAttributeTestHelper(
}

func TestGetOptionalParameters(t *testing.T) {
for _, imuEnabled := range []bool{true, false} {
for _, newConfigFlag := range []bool{true, false} {
for _, cloudStoryEnabled := range []bool{true, false} {
suffix := fmt.Sprintf("with imuIntegrationEnabled = %t & cloudStoryEnabled = %t", imuEnabled, cloudStoryEnabled)
getOptionalParametersTestHelper(t, imuEnabled, cloudStoryEnabled, suffix)
suffix := fmt.Sprintf("with newConfigFlag = %t & cloudStoryEnabled = %t", newConfigFlag, cloudStoryEnabled)
getOptionalParametersTestHelper(t, newConfigFlag, cloudStoryEnabled, suffix)
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,9 @@ func testHelperCartographer(
ConfigParams: map[string]string{
"mode": reflect.ValueOf(subAlgo).String(),
},
MapRateSec: &mapRateSec,
DataDirectory: dataDirectory,
IMUIntegrationEnabled: true,
MapRateSec: &mapRateSec,
DataDirectory: dataDirectory,
NewConfigFlag: true,
}

// Add lidar component to config (required)
Expand Down
2 changes: 1 addition & 1 deletion testhelper/testhelper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func CreateSLAMService(
// feature flag for IMU Integration sets whether to use the dictionary or list format for configuring sensors
cameraName := ""
imuName := ""
if cfg.IMUIntegrationEnabled {
if cfg.NewConfigFlag {
cameraName = cfg.Camera["name"]
imuName = cfg.MovementSensor["name"]
} else {
Expand Down
2 changes: 1 addition & 1 deletion viam_cartographer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func New(

// feature flag for new config
lidarName := ""
if svcConfig.IMUIntegrationEnabled {
if svcConfig.NewConfigFlag {
lidarName = svcConfig.Camera["name"]
} else {
lidarName = svcConfig.Sensors[0]
Expand Down
Loading

0 comments on commit 3e26b32

Please sign in to comment.