Skip to content

Commit

Permalink
[RSDK-3551] remove-feature-flag-delete-dead-code (#207)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nicksanford
nicksanford authored Jul 20, 2023
1 parent b8fcea6 commit 4449617
Show file tree
Hide file tree
Showing 60 changed files with 1,603 additions and 7,705 deletions.
17 changes: 9 additions & 8 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@ endif

build: cartographer-module

viam-cartographer/build/carto_grpc_server: ensure-submodule-initialized grpc/buf
viam-cartographer/build/unit_tests: ensure-submodule-initialized grpc/buf
cd viam-cartographer && cmake -Bbuild -G Ninja ${EXTRA_CMAKE_FLAGS} && cmake --build build

cartographer-module: viam-cartographer/build/carto_grpc_server
cartographer-module: viam-cartographer/build/unit_tests
rm -f bin/cartographer-module
mkdir -p bin && go build $(GO_BUILD_LDFLAGS) -o bin/cartographer-module module/main.go

Expand All @@ -124,19 +124,22 @@ test-cpp:
setup-cpp-full-mod:
sudo apt install -y valgrind gdb

setup-cpp-debug:
sudo apt install -y valgrind gdb

# Linux only
test-cpp-full-mod-asan: build-asan
viam-cartographer/build/unit_tests -p -l all -t CartoFacade_io -t CartoFacadeCPPAPI
viam-cartographer/build/unit_tests -p -l all

test-cpp-full-mod-valgrind: build-debug
valgrind --error-exitcode=1 --leak-check=full -s viam-cartographer/build/unit_tests -p -l all -t CartoFacade_io -t CartoFacadeCPPAPI
valgrind --error-exitcode=1 --leak-check=full -s viam-cartographer/build/unit_tests -p -l all

# Linux only
test-cpp-full-mod-gdb: build-debug
gdb --batch --ex run --ex bt --ex q --args viam-cartographer/build/unit_tests -p -l all -t CartoFacadeCPPAPI
gdb --batch --ex run --ex bt --ex q --args viam-cartographer/build/unit_tests -p -l all

test-cpp-full-mod: build-debug
viam-cartographer/build/unit_tests -p -l all -t CartoFacadeCPPAPI
viam-cartographer/build/unit_tests -p -l all

test-go:
go test -race ./...
Expand All @@ -148,9 +151,7 @@ install-lua-files:
sudo cp viam-cartographer/lua_files/* /usr/local/share/cartographer/lua_files/

install: install-lua-files
sudo rm -f /usr/local/bin/carto_grpc_server
sudo rm -f /usr/local/bin/cartographer-module
sudo cp viam-cartographer/build/carto_grpc_server /usr/local/bin/carto_grpc_server
sudo cp bin/cartographer-module /usr/local/bin/cartographer-module

include *.make
2 changes: 0 additions & 2 deletions cartofacade/capi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package cartofacade

/*
//TODO: check if removing homebrew prefix in cmake causes link to fail
#cgo CFLAGS: -I../viam-cartographer/src/carto_facade
// the libraries that need to be linked can be derived from line 258 of the build.ninja file that is autogenerated during make build
Expand Down Expand Up @@ -262,7 +261,6 @@ func (vc *Carto) getPosition() (GetPosition, error) {

// GetPointCloudMap is a wrapper for viam_carto_get_point_cloud_map
func (vc *Carto) getPointCloudMap() ([]byte, error) {
// TODO: determine whether or not return needs to be a pointer for performance reasons
value := C.viam_carto_get_point_cloud_map_response{}

status := C.viam_carto_get_point_cloud_map(vc.value, &value)
Expand Down
101 changes: 12 additions & 89 deletions config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,66 +12,21 @@ func newError(configError string) error {
return errors.Errorf("SLAM Service configuration error: %s", configError)
}

// DetermineDeleteProcessedData will determine the value of the deleteProcessData attribute
// based on the useLiveData and deleteData input parameters.
func DetermineDeleteProcessedData(logger golog.Logger, deleteData *bool, useLiveData bool) bool {
var deleteProcessedData bool
if deleteData == nil {
deleteProcessedData = useLiveData
} else {
deleteProcessedData = *deleteData
if !useLiveData && deleteProcessedData {
logger.Debug("a value of true cannot be given for delete_processed_data when in offline mode, setting to false")
deleteProcessedData = false
}
}
return deleteProcessedData
}

// DetermineUseLiveData will determine the value of the useLiveData attribute
// based on the liveData input parameter and sensor list.
func DetermineUseLiveData(logger golog.Logger, liveData *bool, sensors []string) (bool, error) {
// If liveData is not set, default it to true and require a sensor to have been provided.
if liveData == nil {
if len(sensors) == 0 {
return false, newError("sensors field cannot be empty")
}
return true, nil
}

useLiveData := *liveData
if useLiveData && len(sensors) == 0 {
return false, newError("sensors field cannot be empty when use_live_data is set to true")
}
return useLiveData, nil
}

// Config describes how to configure the SLAM service.
type Config struct {
Sensors []string `json:"sensors"`
ConfigParams map[string]string `json:"config_params"`
DataDirectory string `json:"data_dir"`
UseLiveData *bool `json:"use_live_data"`
DataRateMsec int `json:"data_rate_msec"`
MapRateSec *int `json:"map_rate_sec"`
Port string `json:"port"`
DeleteProcessedData *bool `json:"delete_processed_data"`
ModularizationV2Enabled *bool `json:"modularization_v2_enabled"`
Sensors []string `json:"sensors"`
ConfigParams map[string]string `json:"config_params"`
DataDirectory string `json:"data_dir"`
DataRateMsec int `json:"data_rate_msec"`
MapRateSec *int `json:"map_rate_sec"`
}

var errSensorsMustNotBeEmpty = errors.New("\"sensors\" must not be empty")

// Validate creates the list of implicit dependencies.
func (config *Config) Validate(path string) ([]string, error) {
// get feature flag provided in config
modularizationV2Enabled := false
if config.ModularizationV2Enabled != nil {
modularizationV2Enabled = *config.ModularizationV2Enabled
}

// require at least one sensor for full mod v2
if modularizationV2Enabled {
if config.Sensors == nil || len(config.Sensors) < 1 {
return nil, utils.NewConfigValidationFieldRequiredError(path, "at least one sensor must be configured")
}
if config.Sensors == nil || len(config.Sensors) < 1 {
return nil, utils.NewConfigValidationError(path, errSensorsMustNotBeEmpty)
}

if config.ConfigParams["mode"] == "" {
Expand All @@ -97,23 +52,9 @@ func (config *Config) Validate(path string) ([]string, error) {

// GetOptionalParameters sets any unset optional config parameters to the values passed to this function,
// and returns them.
func GetOptionalParameters(config *Config, defaultPort string,
func GetOptionalParameters(config *Config,
defaultDataRateMsec, defaultMapRateSec int, logger golog.Logger,
) (string, int, int, bool, bool, bool, error) {
modularizationV2Enabled := false
if config.ModularizationV2Enabled != nil {
modularizationV2Enabled = *config.ModularizationV2Enabled
logger.Debugf("modularization_v2_enabled has been provided, modularization_v2_enabled = %v", modularizationV2Enabled)
}

// Do not set port if mod v2 is enabled. This will be updated during modularization v2.
port := config.Port
if modularizationV2Enabled {
port = defaultPort
} else if config.Port == "" {
port = defaultPort
}

) (int, int) {
dataRateMsec := config.DataRateMsec
if config.DataRateMsec == 0 {
dataRateMsec = defaultDataRateMsec
Expand All @@ -127,24 +68,6 @@ func GetOptionalParameters(config *Config, defaultPort string,
} else {
mapRateSec = *config.MapRateSec
}
if mapRateSec == 0 {
logger.Info("setting slam system to localization mode")
}

var err error
useLiveData := true
if !modularizationV2Enabled {
useLiveData, err = DetermineUseLiveData(logger, config.UseLiveData, config.Sensors)
if err != nil {
return "", 0, 0, false, false, false, err
}
}

// only validate deleteProcessedData if mod v2 is not enabled
deleteProcessedData := false
if !modularizationV2Enabled {
deleteProcessedData = DetermineDeleteProcessedData(logger, config.DeleteProcessedData, useLiveData)
}

return port, dataRateMsec, mapRateSec, useLiveData, deleteProcessedData, modularizationV2Enabled, nil
return dataRateMsec, mapRateSec
}
118 changes: 28 additions & 90 deletions config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@ import (
"go.viam.com/utils"
)

var (
_true = true
_false = false
)

func TestValidate(t *testing.T) {
testCfgPath := "services.slam.attributes.fake"
logger := golog.NewTestLogger(t)
Expand All @@ -23,7 +18,7 @@ func TestValidate(t *testing.T) {
model := resource.DefaultModelFamily.WithModel("test")
cfgService := resource.Config{Name: "test", API: slam.API, Model: model}
_, err := newConfig(cfgService)
test.That(t, err, test.ShouldBeError, newError("error validating \"services.slam.attributes.fake\": \"config_params[mode]\" is required"))
test.That(t, err, test.ShouldBeError, newError("error validating \"services.slam.attributes.fake\": \"sensors\" must not be empty"))
})

t.Run("Simplest valid config", func(t *testing.T) {
Expand All @@ -33,16 +28,21 @@ func TestValidate(t *testing.T) {
})

t.Run("Config without required fields", func(t *testing.T) {
// Test for missing main attribute fields
requiredFields := []string{"data_dir"}
for _, requiredField := range requiredFields {
requiredFields := []string{"data_dir", "sensors"}
dataDirErr := utils.NewConfigValidationFieldRequiredError(testCfgPath, requiredFields[0])
sensorsErr := utils.NewConfigValidationError(testCfgPath, errSensorsMustNotBeEmpty)

expectedErrors := []error{
newError(dataDirErr.Error()),
newError(sensorsErr.Error()),
}
for i, requiredField := range requiredFields {
logger.Debugf("Testing SLAM config without %s\n", requiredField)
cfgService := makeCfgService()
delete(cfgService.Attributes, requiredField)
_, err := newConfig(cfgService)
expected := newError(utils.NewConfigValidationFieldRequiredError(testCfgPath, requiredField).Error())

test.That(t, err, test.ShouldBeError, expected)
test.That(t, err, test.ShouldBeError, expectedErrors[i])
}
// Test for missing config_params attributes
logger.Debug("Testing SLAM config without config_params[mode]")
Expand All @@ -54,11 +54,11 @@ func TestValidate(t *testing.T) {

t.Run("Config with invalid parameter type", func(t *testing.T) {
cfgService := makeCfgService()
cfgService.Attributes["use_live_data"] = "true"
cfgService.Attributes["data_dir"] = true
_, err := newConfig(cfgService)
expE := newError("1 error(s) decoding:\n\n* 'use_live_data' expected type 'bool', got unconvertible type 'string', value: 'true'")
expE := newError("1 error(s) decoding:\n\n* 'data_dir' expected type 'string', got unconvertible type 'bool', value: 'true'")
test.That(t, err, test.ShouldBeError, expE)
cfgService.Attributes["use_live_data"] = true
cfgService.Attributes["data_dir"] = "true"
_, err = newConfig(cfgService)
test.That(t, err, test.ShouldBeNil)
})
Expand All @@ -79,8 +79,6 @@ func TestValidate(t *testing.T) {
cfgService.Attributes["sensors"] = []string{"a", "b"}
cfgService.Attributes["data_rate_msec"] = 1001
cfgService.Attributes["map_rate_sec"] = 1002
cfgService.Attributes["port"] = "47"
cfgService.Attributes["delete_processed_data"] = true

cfgService.Attributes["config_params"] = map[string]string{
"mode": "test mode",
Expand All @@ -90,74 +88,13 @@ func TestValidate(t *testing.T) {
cfg, err := newConfig(cfgService)
test.That(t, err, test.ShouldBeNil)
test.That(t, cfg.DataDirectory, test.ShouldEqual, cfgService.Attributes["data_dir"])
test.That(t, *cfg.UseLiveData, test.ShouldEqual, cfgService.Attributes["use_live_data"])
test.That(t, cfg.Sensors, test.ShouldResemble, cfgService.Attributes["sensors"])
test.That(t, cfg.DataRateMsec, test.ShouldEqual, cfgService.Attributes["data_rate_msec"])
test.That(t, *cfg.MapRateSec, test.ShouldEqual, cfgService.Attributes["map_rate_sec"])
test.That(t, cfg.Port, test.ShouldEqual, cfgService.Attributes["port"])
test.That(t, cfg.ConfigParams, test.ShouldResemble, cfgService.Attributes["config_params"])
})
}

func TestDetermineDeleteProcessedData(t *testing.T) {
logger := golog.NewTestLogger(t)

t.Run("No delete_processed_data provided", func(t *testing.T) {
deleteProcessedData := DetermineDeleteProcessedData(logger, nil, false)
test.That(t, deleteProcessedData, test.ShouldBeFalse)

deleteProcessedData = DetermineDeleteProcessedData(logger, nil, true)
test.That(t, deleteProcessedData, test.ShouldBeTrue)
})

t.Run("False delete_processed_data", func(t *testing.T) {
deleteProcessedData := DetermineDeleteProcessedData(logger, &_false, false)
test.That(t, deleteProcessedData, test.ShouldBeFalse)

deleteProcessedData = DetermineDeleteProcessedData(logger, &_false, true)
test.That(t, deleteProcessedData, test.ShouldBeFalse)
})

t.Run("True delete_processed_data", func(t *testing.T) {
deleteProcessedData := DetermineDeleteProcessedData(logger, &_true, false)
test.That(t, deleteProcessedData, test.ShouldBeFalse)

deleteProcessedData = DetermineDeleteProcessedData(logger, &_true, true)
test.That(t, deleteProcessedData, test.ShouldBeTrue)
})
}

func TestDetermineUseLiveData(t *testing.T) {
logger := golog.NewTestLogger(t)
t.Run("No use_live_data specified", func(t *testing.T) {
useLiveData, err := DetermineUseLiveData(logger, nil, []string{})
test.That(t, err, test.ShouldBeError, newError("sensors field cannot be empty"))
test.That(t, useLiveData, test.ShouldBeFalse)

useLiveData, err = DetermineUseLiveData(logger, nil, []string{"camera"})
test.That(t, err, test.ShouldBeNil)
test.That(t, useLiveData, test.ShouldBeTrue)
})
t.Run("False use_live_data", func(t *testing.T) {
useLiveData, err := DetermineUseLiveData(logger, &_false, []string{})
test.That(t, err, test.ShouldBeNil)
test.That(t, useLiveData, test.ShouldBeFalse)

useLiveData, err = DetermineUseLiveData(logger, &_false, []string{"camera"})
test.That(t, err, test.ShouldBeNil)
test.That(t, useLiveData, test.ShouldBeFalse)
})
t.Run("True use_live_data", func(t *testing.T) {
useLiveData, err := DetermineUseLiveData(logger, &_true, []string{})
test.That(t, err, test.ShouldBeError, newError("sensors field cannot be empty when use_live_data is set to true"))
test.That(t, useLiveData, test.ShouldBeFalse)

useLiveData, err = DetermineUseLiveData(logger, &_true, []string{"camera"})
test.That(t, err, test.ShouldBeNil)
test.That(t, useLiveData, test.ShouldBeTrue)
})
}

// makeCfgService creates the simplest possible config that can pass validation.
func makeCfgService() resource.Config {
model := resource.DefaultModelFamily.WithModel("test")
Expand All @@ -167,7 +104,7 @@ func makeCfgService() resource.Config {
"mode": "test mode",
}
cfgService.Attributes["data_dir"] = "path"
cfgService.Attributes["use_live_data"] = true
cfgService.Attributes["sensors"] = []string{"a"}
return cfgService
}

Expand All @@ -177,31 +114,32 @@ func TestGetOptionalParameters(t *testing.T) {
t.Run("Pass default parameters", func(t *testing.T) {
cfgService := makeCfgService()
cfgService.Attributes["sensors"] = []string{"a"}
cfgService.Attributes["use_live_data"] = true
cfg, err := newConfig(cfgService)
test.That(t, err, test.ShouldBeNil)
port, dataRateMsec, mapRateSec, useLiveData, deleteProcessedData, modularizationV2Enabled, err := GetOptionalParameters(
dataRateMsec, mapRateSec := GetOptionalParameters(
cfg,
"localhost",
1001,
1002,
logger)
test.That(t, err, test.ShouldBeNil)
test.That(t, port, test.ShouldResemble, "localhost")
test.That(t, dataRateMsec, test.ShouldEqual, 1001)
test.That(t, mapRateSec, test.ShouldEqual, 1002)
test.That(t, useLiveData, test.ShouldEqual, true)
test.That(t, deleteProcessedData, test.ShouldEqual, true)
test.That(t, modularizationV2Enabled, test.ShouldEqual, false)
})

t.Run("Live data without sensors", func(t *testing.T) {
t.Run("Return overrides", func(t *testing.T) {
cfgService := makeCfgService()
cfgService.Attributes["use_live_data"] = true
cfgService.Attributes["sensors"] = []string{"a"}
cfg, err := newConfig(cfgService)
two := 2
cfg.DataRateMsec = 1
cfg.MapRateSec = &two
test.That(t, err, test.ShouldBeNil)
_, _, _, _, _, _, err = GetOptionalParameters(cfg, "localhost", 1001, 1002, logger)
test.That(t, err, test.ShouldBeError, newError("sensors field cannot be empty when use_live_data is set to true"))
dataRateMsec, mapRateSec := GetOptionalParameters(
cfg,
1001,
1002,
logger)
test.That(t, dataRateMsec, test.ShouldEqual, 1)
test.That(t, mapRateSec, test.ShouldEqual, 2)
})
}

Expand Down
Loading

0 comments on commit 4449617

Please sign in to comment.