Params refactor, simplified (#2300)

* always c++

* Create C++ params class

* get works

* tests hang now

* passes tests

* cleanup string formatting

* handle interrupt in blocking read

* fix memory leak

* remove unused constructor

* Use delete_db_value directly

* Rename put -> write_db_value

* filename cleanup

* no semicolons in cython

* Update common/SConscript

Co-authored-by: Adeeb Shihadeh <adeebshihadeh@gmail.com>

* add std::string version of delete_db_value

* This is handled

* cleanup encoding

* Add clear method to clear all

* add persistent params

* fix android build

* Should be called clear_all

* only import params when needed

* set params path on manager import

* recusrively create directories

* Fix function order

* cleanup mkdirp

Co-authored-by: Adeeb Shihadeh <adeebshihadeh@gmail.com>
Co-authored-by: Comma Device <device@comma.ai>
old-commit-hash: 2e182e5c57
This commit is contained in:
Willem Melching
2020-10-13 16:23:23 +02:00
committed by GitHub
parent a331861873
commit 498ecbdf3c
20 changed files with 417 additions and 577 deletions
+10 -7
View File
@@ -71,7 +71,7 @@ void safety_setter_thread() {
return;
};
std::vector<char> value_vin = read_db_bytes("CarVin");
std::vector<char> value_vin = Params().read_db_bytes("CarVin");
if (value_vin.size() > 0) {
// sanity check VIN format
assert(value_vin.size() == 17);
@@ -93,7 +93,7 @@ void safety_setter_thread() {
return;
};
params = read_db_bytes("CarParams");
params = Params().read_db_bytes("CarParams");
if (params.size() > 0) break;
usleep(100*1000);
}
@@ -124,13 +124,15 @@ bool usb_connect() {
return false;
}
Params params = Params();
if (getenv("BOARDD_LOOPBACK")) {
panda->set_loopback(true);
}
const char *fw_sig_buf = panda->get_firmware_version();
if (fw_sig_buf){
write_db_value("PandaFirmware", fw_sig_buf, 128);
params.write_db_value("PandaFirmware", fw_sig_buf, 128);
// Convert to hex for offroad
char fw_sig_hex_buf[16] = {0};
@@ -139,7 +141,7 @@ bool usb_connect() {
fw_sig_hex_buf[2*i+1] = NIBBLE_TO_HEX((uint8_t)fw_sig_buf[i] & 0xF);
}
write_db_value("PandaFirmwareHex", fw_sig_hex_buf, 16);
params.write_db_value("PandaFirmwareHex", fw_sig_hex_buf, 16);
LOGW("fw signature: %.*s", 16, fw_sig_hex_buf);
delete[] fw_sig_buf;
@@ -150,7 +152,7 @@ bool usb_connect() {
if (serial_buf) {
size_t serial_sz = strnlen(serial_buf, 16);
write_db_value("PandaDongleId", serial_buf, serial_sz);
params.write_db_value("PandaDongleId", serial_buf, serial_sz);
LOGW("panda serial: %.*s", serial_sz, serial_buf);
delete[] serial_buf;
@@ -269,6 +271,7 @@ void can_health_thread() {
uint32_t no_ignition_cnt = 0;
bool ignition_last = false;
Params params = Params();
// Broadcast empty health message when panda is not yet connected
while (!panda){
@@ -318,9 +321,9 @@ void can_health_thread() {
// clear VIN, CarParams, and set new safety on car start
if (ignition && !ignition_last) {
int result = delete_db_value("CarVin");
int result = params.delete_db_value("CarVin");
assert((result == 0) || (result == ERR_NO_VALUE));
result = delete_db_value("CarParams");
result = params.delete_db_value("CarParams");
assert((result == 0) || (result == ERR_NO_VALUE));
if (!safety_setter_thread_running) {
+2 -2
View File
@@ -183,7 +183,7 @@ struct VisionState {
void* frontview_thread(void *arg) {
int err;
VisionState *s = (VisionState*)arg;
s->rhd_front = read_db_bool("IsRHD");
s->rhd_front = Params().read_db_bool("IsRHD");
set_thread_name("frontview");
err = set_realtime_priority(51);
@@ -1538,7 +1538,7 @@ void free_buffers(VisionState *s) {
clReleaseProgram(s->prg_rgb_laplacian);
clReleaseKernel(s->krnl_rgb_laplacian);
}
void party(VisionState *s) {
+119 -100
View File
@@ -13,6 +13,8 @@
#include <map>
#include <string>
#include <iostream>
#include <csignal>
#include <string.h>
#include "common/util.h"
@@ -37,9 +39,15 @@ static const char* persistent_params_path = default_params_path;
} //namespace
volatile sig_atomic_t params_do_exit = 0;
void params_sig_handler(int signal) {
std::cout << "got signal" << std::endl;
params_do_exit = 1;
}
static int fsync_dir(const char* path){
int result = 0;
int fd = open(path, O_RDONLY);
int fd = open(path, O_RDONLY, 0755);
if (fd < 0){
result = -1;
@@ -64,15 +72,44 @@ static int fsync_dir(const char* path){
}
}
static int ensure_dir_exists(const char* path) {
struct stat st;
if (stat(path, &st) == -1) {
return mkdir(path, 0700);
static int mkdir_p(std::string path) {
char * _path = (char *)path.c_str();
for (char *p = _path + 1; *p; p++) {
if (*p == '/') {
*p = '\0'; // Temporarily truncate
if (mkdir(_path, 0775) != 0) {
if (errno != EEXIST) return -1;
}
*p = '/';
}
}
if (mkdir(_path, 0775) != 0) {
if (errno != EEXIST) return -1;
}
return 0;
}
int write_db_value(const char* key, const char* value, size_t value_size, bool persistent_param) {
static int ensure_dir_exists(std::string path) {
// TODO: replace by std::filesystem::create_directories
return mkdir_p(path.c_str());
}
Params::Params(bool persistent_param){
const char * path = persistent_param ? persistent_params_path : default_params_path;
params_path = std::string(path);
}
Params::Params(std::string path) {
params_path = path;
}
int Params::write_db_value(std::string key, std::string dat){
return write_db_value(key.c_str(), dat.c_str(), dat.length());
}
int Params::write_db_value(const char* key, const char* value, size_t value_size) {
// Information about safely and atomically writing a file: https://lwn.net/Articles/457667/
// 1) Create temp file
// 2) Write data to temp file
@@ -83,11 +120,9 @@ int write_db_value(const char* key, const char* value, size_t value_size, bool p
int lock_fd = -1;
int tmp_fd = -1;
int result;
char tmp_path[1024];
char path[1024];
char *tmp_dir;
std::string path;
std::string tmp_path;
ssize_t bytes_written;
const char* params_path = persistent_param ? persistent_params_path : default_params_path;
// Make sure params path exists
result = ensure_dir_exists(params_path);
@@ -95,59 +130,43 @@ int write_db_value(const char* key, const char* value, size_t value_size, bool p
goto cleanup;
}
result = snprintf(path, sizeof(path), "%s/d", params_path);
if (result < 0) {
goto cleanup;
}
// See if the symlink exists, otherwise create it
path = params_path + "/d";
struct stat st;
if (stat(path, &st) == -1) {
if (stat(path.c_str(), &st) == -1) {
// Create temp folder
result = snprintf(path, sizeof(path), "%s/.tmp_XXXXXX", params_path);
if (result < 0) {
goto cleanup;
}
tmp_dir = mkdtemp(path);
if (tmp_dir == NULL){
path = params_path + "/.tmp_XXXXXX";
char *t = mkdtemp((char*)path.c_str());
if (t == NULL){
goto cleanup;
}
std::string tmp_dir(t);
// Set permissions
result = chmod(tmp_dir, 0777);
result = chmod(tmp_dir.c_str(), 0777);
if (result < 0) {
goto cleanup;
}
// Symlink it to temp link
result = snprintf(tmp_path, sizeof(tmp_path), "%s.link", tmp_dir);
if (result < 0) {
goto cleanup;
}
result = symlink(tmp_dir, tmp_path);
tmp_path = tmp_dir + ".link";
result = symlink(tmp_dir.c_str(), tmp_path.c_str());
if (result < 0) {
goto cleanup;
}
// Move symlink to <params>/d
result = snprintf(path, sizeof(path), "%s/d", params_path);
if (result < 0) {
goto cleanup;
}
result = rename(tmp_path, path);
path = params_path + "/d";
result = rename(tmp_path.c_str(), path.c_str());
if (result < 0) {
goto cleanup;
}
}
// Write value to temp.
result =
snprintf(tmp_path, sizeof(tmp_path), "%s/.tmp_value_XXXXXX", params_path);
if (result < 0) {
goto cleanup;
}
tmp_fd = mkstemp(tmp_path);
tmp_path = params_path + "/.tmp_value_XXXXXX";
tmp_fd = mkstemp((char*)tmp_path.c_str());
bytes_written = write(tmp_fd, value, value_size);
if (bytes_written != value_size) {
result = -20;
@@ -155,17 +174,11 @@ int write_db_value(const char* key, const char* value, size_t value_size, bool p
}
// Build lock path
result = snprintf(path, sizeof(path), "%s/.lock", params_path);
if (result < 0) {
goto cleanup;
}
lock_fd = open(path, O_CREAT);
path = params_path + "/.lock";
lock_fd = open(path.c_str(), O_CREAT, 0775);
// Build key path
result = snprintf(path, sizeof(path), "%s/d/%s", params_path, key);
if (result < 0) {
goto cleanup;
}
path = params_path + "/d/" + std::string(key);
// Take lock.
result = flock(lock_fd, LOCK_EX);
@@ -186,18 +199,14 @@ int write_db_value(const char* key, const char* value, size_t value_size, bool p
}
// Move temp into place.
result = rename(tmp_path, path);
result = rename(tmp_path.c_str(), path.c_str());
if (result < 0) {
goto cleanup;
}
// fsync parent directory
result = snprintf(path, sizeof(path), "%s/d", params_path);
if (result < 0) {
goto cleanup;
}
result = fsync_dir(path);
path = params_path + "/d";
result = fsync_dir(path.c_str());
if (result < 0) {
goto cleanup;
}
@@ -209,25 +218,25 @@ cleanup:
}
if (tmp_fd >= 0) {
if (result < 0) {
remove(tmp_path);
remove(tmp_path.c_str());
}
close(tmp_fd);
}
return result;
}
int delete_db_value(const char* key, bool persistent_param) {
int Params::delete_db_value(std::string key) {
return delete_db_value(key.c_str());
}
int Params::delete_db_value(const char* key) {
int lock_fd = -1;
int result;
char path[1024];
const char* params_path = persistent_param ? persistent_params_path : default_params_path;
std::string path;
// Build lock path, and open lockfile
result = snprintf(path, sizeof(path), "%s/.lock", params_path);
if (result < 0) {
goto cleanup;
}
lock_fd = open(path, O_CREAT);
path = params_path + "/.lock";
lock_fd = open(path.c_str(), O_CREAT, 0775);
// Take lock.
result = flock(lock_fd, LOCK_EX);
@@ -235,26 +244,17 @@ int delete_db_value(const char* key, bool persistent_param) {
goto cleanup;
}
// Build key path
result = snprintf(path, sizeof(path), "%s/d/%s", params_path, key);
if (result < 0) {
goto cleanup;
}
// Delete value.
result = remove(path);
path = params_path + "/d/" + std::string(key);
result = remove(path.c_str());
if (result != 0) {
result = ERR_NO_VALUE;
goto cleanup;
}
// fsync parent directory
result = snprintf(path, sizeof(path), "%s/d", params_path);
if (result < 0) {
goto cleanup;
}
result = fsync_dir(path);
path = params_path + "/d";
result = fsync_dir(path.c_str());
if (result < 0) {
goto cleanup;
}
@@ -267,39 +267,58 @@ cleanup:
return result;
}
int read_db_value(const char* key, char** value, size_t* value_sz, bool persistent_param) {
char path[1024];
const char* params_path = persistent_param ? persistent_params_path : default_params_path;
std::string Params::get(std::string key, bool block){
char* value;
size_t size;
int r;
int result = snprintf(path, sizeof(path), "%s/d/%s", params_path, key);
if (result < 0) {
return result;
if (block){
r = read_db_value_blocking((const char*)key.c_str(), &value, &size);
} else {
r = read_db_value((const char*)key.c_str(), &value, &size);
}
*value = static_cast<char*>(read_file(path, value_sz));
if (r == 0){
std::string s(value, size);
free(value);
return s;
} else {
return "";
}
}
int Params::read_db_value(const char* key, char** value, size_t* value_sz) {
std::string path = params_path + "/d/" + std::string(key);
*value = static_cast<char*>(read_file(path.c_str(), value_sz));
if (*value == NULL) {
return -22;
}
return 0;
}
void read_db_value_blocking(const char* key, char** value, size_t* value_sz, bool persistent_param) {
while (1) {
const int result = read_db_value(key, value, value_sz, persistent_param);
int Params::read_db_value_blocking(const char* key, char** value, size_t* value_sz) {
params_do_exit = 0;
void (*prev_handler_sigint)(int) = std::signal(SIGINT, params_sig_handler);
void (*prev_handler_sigterm)(int) = std::signal(SIGTERM, params_sig_handler);
while (!params_do_exit) {
const int result = read_db_value(key, value, value_sz);
if (result == 0) {
return;
break;
} else {
// Sleep for 0.1 seconds.
usleep(100000);
usleep(100000); // 0.1 s
}
}
std::signal(SIGINT, prev_handler_sigint);
std::signal(SIGTERM, prev_handler_sigterm);
return params_do_exit; // Return 0 if we had no interrupt
}
int read_db_all(std::map<std::string, std::string> *params, bool persistent_param) {
int Params::read_db_all(std::map<std::string, std::string> *params) {
int err = 0;
const char* params_path = persistent_param ? persistent_params_path : default_params_path;
std::string lock_path = util::string_format("%s/.lock", params_path);
std::string lock_path = params_path + "/.lock";
int lock_fd = open(lock_path.c_str(), 0);
if (lock_fd < 0) return -1;
@@ -310,7 +329,7 @@ int read_db_all(std::map<std::string, std::string> *params, bool persistent_para
return err;
}
std::string key_path = util::string_format("%s/d", params_path);
std::string key_path = params_path + "/d";
DIR *d = opendir(key_path.c_str());
if (!d) {
close(lock_fd);
@@ -321,7 +340,7 @@ int read_db_all(std::map<std::string, std::string> *params, bool persistent_para
while ((de = readdir(d))) {
if (!isalnum(de->d_name[0])) continue;
std::string key = std::string(de->d_name);
std::string value = util::read_file(util::string_format("%s/%s", key_path.c_str(), key.c_str()));
std::string value = util::read_file(key_path + "/" + key);
(*params)[key] = value;
}
@@ -332,11 +351,11 @@ int read_db_all(std::map<std::string, std::string> *params, bool persistent_para
return 0;
}
std::vector<char> read_db_bytes(const char* param_name, bool persistent_param) {
std::vector<char> Params::read_db_bytes(const char* param_name) {
std::vector<char> bytes;
char* value;
size_t sz;
int result = read_db_value(param_name, &value, &sz, persistent_param);
int result = read_db_value(param_name, &value, &sz);
if (result == 0) {
bytes.assign(value, value+sz);
free(value);
@@ -344,7 +363,7 @@ std::vector<char> read_db_bytes(const char* param_name, bool persistent_param) {
return bytes;
}
bool read_db_bool(const char* param_name, bool persistent_param) {
std::vector<char> bytes = read_db_bytes(param_name, persistent_param);
bool Params::read_db_bool(const char* param_name) {
std::vector<char> bytes = read_db_bytes(param_name);
return bytes.size() > 0 and bytes[0] == '1';
}
+42 -39
View File
@@ -1,44 +1,47 @@
#pragma once
#include <stddef.h>
#ifdef __cplusplus
extern "C" {
#endif
#define ERR_NO_VALUE -33
int write_db_value(const char* key, const char* value, size_t value_size, bool persistent_param = false);
// Reads a value from the params database.
// Inputs:
// key: The key to read.
// value: A pointer where a newly allocated string containing the db value will
// be written.
// value_sz: A pointer where the size of value will be written. Does not
// include the NULL terminator.
// persistent_param: Boolean indicating if the param store in the /persist partition is to be used.
// e.g. for sensor calibration files. Will not be cleared after wipe or re-install.
//
// Returns: Negative on failure, otherwise 0.
int read_db_value(const char* key, char** value, size_t* value_sz, bool persistent_param = false);
// Delete a value from the params database.
// Inputs are the same as read_db_value, without value and value_sz.
int delete_db_value(const char* key, bool persistent_param = false);
// Reads a value from the params database, blocking until successful.
// Inputs are the same as read_db_value.
void read_db_value_blocking(const char* key, char** value, size_t* value_sz, bool persistent_param = false);
#ifdef __cplusplus
} // extern "C"
#endif
#ifdef __cplusplus
#include <map>
#include <string>
#include <vector>
int read_db_all(std::map<std::string, std::string> *params, bool persistent_param = false);
std::vector<char> read_db_bytes(const char* param_name, bool persistent_param = false);
bool read_db_bool(const char* param_name, bool persistent_param = false);
#endif
#define ERR_NO_VALUE -33
class Params {
private:
std::string params_path;
public:
Params(bool persistent_param = false);
Params(std::string path);
int write_db_value(std::string key, std::string dat);
int write_db_value(const char* key, const char* value, size_t value_size);
// Reads a value from the params database.
// Inputs:
// key: The key to read.
// value: A pointer where a newly allocated string containing the db value will
// be written.
// value_sz: A pointer where the size of value will be written. Does not
// include the NULL terminator.
// persistent_param: Boolean indicating if the param store in the /persist partition is to be used.
// e.g. for sensor calibration files. Will not be cleared after wipe or re-install.
//
// Returns: Negative on failure, otherwise 0.
int read_db_value(const char* key, char** value, size_t* value_sz);
// Delete a value from the params database.
// Inputs are the same as read_db_value, without value and value_sz.
int delete_db_value(std::string key);
int delete_db_value(const char* key);
// Reads a value from the params database, blocking until successful.
// Inputs are the same as read_db_value.
int read_db_value_blocking(const char* key, char** value, size_t* value_sz);
int read_db_all(std::map<std::string, std::string> *params);
std::vector<char> read_db_bytes(const char* param_name);
bool read_db_bool(const char* param_name);
std::string get(std::string key, bool block=false);
};
+10 -9
View File
@@ -522,30 +522,31 @@ kj::Array<capnp::word> gen_init_data() {
if (!clean) {
init.setDirty(true);
}
Params params = Params();
std::vector<char> git_commit = read_db_bytes("GitCommit");
std::vector<char> git_commit = params.read_db_bytes("GitCommit");
if (git_commit.size() > 0) {
init.setGitCommit(capnp::Text::Reader(git_commit.data(), git_commit.size()));
}
std::vector<char> git_branch = read_db_bytes("GitBranch");
std::vector<char> git_branch = params.read_db_bytes("GitBranch");
if (git_branch.size() > 0) {
init.setGitBranch(capnp::Text::Reader(git_branch.data(), git_branch.size()));
}
std::vector<char> git_remote = read_db_bytes("GitRemote");
std::vector<char> git_remote = params.read_db_bytes("GitRemote");
if (git_remote.size() > 0) {
init.setGitRemote(capnp::Text::Reader(git_remote.data(), git_remote.size()));
}
init.setPassive(read_db_bool("Passive"));
init.setPassive(params.read_db_bool("Passive"));
{
// log params
std::map<std::string, std::string> params;
read_db_all(&params);
auto lparams = init.initParams().initEntries(params.size());
std::map<std::string, std::string> params_map;
params.read_db_all(&params_map);
auto lparams = init.initParams().initEntries(params_map.size());
int i = 0;
for (auto& kv : params) {
for (auto& kv : params_map) {
auto lentry = lparams[i];
lentry.setKey(kv.first);
lentry.setValue(kv.second);
@@ -613,7 +614,7 @@ int main(int argc, char** argv) {
}
bool record_front = true;
#ifndef QCOM2
record_front = read_db_bool("RecordFront");
record_front = Params().read_db_bool("RecordFront");
#endif
setpriority(PRIO_PROCESS, 0, -12);
+1 -2
View File
@@ -18,6 +18,7 @@ from common.hardware import HARDWARE, ANDROID, PC
WEBCAM = os.getenv("WEBCAM") is not None
sys.path.append(os.path.join(BASEDIR, "pyextra"))
os.environ['BASEDIR'] = BASEDIR
os.environ['PARAMS_PATH'] = PARAMS
TOTAL_SCONS_NODES = 1005
prebuilt = os.path.exists(os.path.join(BASEDIR, 'prebuilt'))
@@ -536,8 +537,6 @@ def uninstall():
HARDWARE.reboot(reason="recovery")
def main():
os.environ['PARAMS_PATH'] = PARAMS
if ANDROID:
# the flippening!
os.system('LD_LIBRARY_PATH="" content insert --uri content://settings/system --bind name:s:user_rotation --bind value:i:1')
+1 -1
View File
@@ -28,7 +28,7 @@ void dmonitoring_init(DMonitoringModelState* s) {
int runtime = USE_DSP_RUNTIME;
#endif
s->m = new DefaultRunModel(model_path, (float*)&s->output, OUTPUT_SIZE, runtime);
s->is_rhd = read_db_bool("IsRHD");
s->is_rhd = Params().read_db_bool("IsRHD");
}
template <class T>
+1 -1
View File
@@ -54,7 +54,7 @@ void model_init(ModelState* s, cl_device_id device_id, cl_context context, int t
s->traffic_convention = std::make_unique<float[]>(TRAFFIC_CONVENTION_LEN);
s->m->addTrafficConvention(s->traffic_convention.get(), TRAFFIC_CONVENTION_LEN);
bool is_rhd = read_db_bool("IsRHD");
bool is_rhd = Params().read_db_bool("IsRHD");
if (is_rhd) {
s->traffic_convention[1] = 1.0;
} else {
+1 -1
View File
@@ -5,11 +5,11 @@ from nose.tools import nottest
from common.hardware import PC
from common.apk import update_apks, start_offroad, pm_apply_packages, android_packages
from common.params import Params
from selfdrive.version import training_version, terms_version
from selfdrive.manager import start_managed_process, kill_managed_process, get_running
def set_params_enabled():
from common.params import Params
params = Params()
params.put("HasAcceptedTerms", terms_version)
params.put("HasCompletedSetup", "1")
+1 -1
View File
@@ -79,7 +79,7 @@ static void handle_vision_touch(UIState *s, int touch_x, int touch_y) {
if (!s->scene.frontview) {
s->scene.uilayout_sidebarcollapsed = !s->scene.uilayout_sidebarcollapsed;
} else {
write_db_value("IsDriverViewEnabled", "0", 1);
Params().write_db_value("IsDriverViewEnabled", "0", 1);
}
}
}
+2 -2
View File
@@ -43,7 +43,7 @@ ParamsToggle::ParamsToggle(QString param, QString title, QString description, QS
setLayout(hlayout);
checkbox->setChecked(read_db_bool(param.toStdString().c_str()));
checkbox->setChecked(Params().read_db_bool(param.toStdString().c_str()));
setStyleSheet(R"(
QCheckBox { font-size: 70px }
@@ -58,7 +58,7 @@ ParamsToggle::ParamsToggle(QString param, QString title, QString description, QS
void ParamsToggle::checkboxClicked(int state){
char value = state ? '1': '0';
write_db_value(param.toStdString().c_str(), &value, 1);
Params().write_db_value(param.toStdString().c_str(), &value, 1);
}
SettingsWindow::SettingsWindow(QWidget *parent) : QWidget(parent) {
+1 -1
View File
@@ -19,7 +19,7 @@ extern volatile sig_atomic_t do_exit;
int write_param_float(float param, const char* param_name, bool persistent_param) {
char s[16];
int size = snprintf(s, sizeof(s), "%f", param);
return write_db_value(param_name, s, size < sizeof(s) ? size : sizeof(s), persistent_param);
return Params(persistent_param).write_db_value(param_name, s, size < sizeof(s) ? size : sizeof(s));
}
void ui_init(UIState *s) {
+1 -1
View File
@@ -204,7 +204,7 @@ int read_param(T* param, const char *param_name, bool persistent_param = false){
char *value;
size_t sz;
int result = read_db_value(param_name, &value, &sz, persistent_param);
int result = Params(persistent_param).read_db_value(param_name, &value, &sz);
if (result == 0){
std::string s = std::string(value, sz); // value is not null terminated
free(value);