Hardware Configuration Manager #369

BillThePlatypus · 2019-12-07T00:58:02Z

This allows for hardware various hardware settings to be chosen, such as where things are plugged in, or which sensors are enabled.

BillThePlatypus · 2019-12-07T01:06:27Z

As a bit of an explanation: Several "devices" are defined, such as the serial connection, airspeed sensor, etc. For each device, there is a choice of several configurations. For example serial connection (device 0) can be over VCP (configuration 0), or UART (configurations 1-3). The airspeed sensor (device 1, I think) can be disabled(configuration 0), or connected to the Flexi port (configuration 1).

Configuration settings are saved at the same time as parameters. Currently this is the /param_write service, but I plan on changing the name to reflect that it saves configurations as well.

All devices default to configuration 0, which are/will be defined to "safe" settings.

Configurations are currently only applied at boot. Currently, there are no checks for invalid configurations (such as GNSS and Airspeed both on the Flexi Port), but those could be added at the board level. There are currently no checks when a configuration is chosen that such a configuration even exists. In rosflight_io, I have left some room for adding verbose error messages.

superjax

This is awesome man! Thanks for all the hard work

This was a monster PR. I think that this could have been split up into at least 3 changes that would have been a lot easier to review.

Biggest thing is actually const correct-ness. A couple of other minor architectural changes from me.

Thanks again!

boards/airbourne/Makefile

boards/airbourne/airbourne_board.cpp

boards/airbourne/airbourne_board.h

include/configuration_enum.h

superjax · 2020-01-09T03:01:09Z

include/param.h

@@ -225,8 +232,6 @@ class Params

 public:
  static constexpr uint8_t PARAMS_NAME_LENGTH = 16;
-
-private:
  union param_value_t


why did this need to become public?

Did I make this public? Not a big deal, but I tend to make constants public. Especially here, because the parameter names pass through other classes, such as CommLink.

Yeah, but they don't need to see this union; that's an internal implementation thing. I'd revert this to private

OK, I see now that I'm looking at the right page on Github.

I needed to make the params_t struct public, so that the memory manager could hold one. Strangely, param_value_t does not need to be public for that to work, even though it is a member of params_t

superjax · 2020-01-09T03:02:03Z

scripts/rosflight.mk

-                nanoprintf.cpp
+                nanoprintf.cpp \
+		config_manager.cpp \
+		memory_manager.cpp \


superjax · 2020-01-09T03:04:36Z

src/sensors.cpp

@@ -78,6 +79,8 @@ void Sensors::init()
  diff_outlier_filt_.init(DIFF_MAX_CHANGE_RATE, DIFF_SAMPLE_RATE, 0.0f);
  sonar_outlier_filt_.init(SONAR_MAX_CHANGE_RATE, SONAR_SAMPLE_RATE, 0.0f);
  int_start_us_ = rf_.board_.clock_micros();
+
+  this->update_battery_monitor_multipliers();


Is there some inheritance I missing?

if not, then just update_battery_monitor_multipliers(); should work

I do that sometimes just to be explicit, but it would look more consistent without it.

superjax · 2020-01-09T03:05:18Z

src/sensors.cpp

@@ -106,6 +109,10 @@ void Sensors::param_change_callback(uint16_t param_id)
  case PARAM_FC_YAW:
    init_imu();
    break;
+  case PARAM_BATTERY_VOLTAGE_MULTIPLIER:
+  case PARAM_BATTERY_CURRENT_MULTIPLIER:
+    this->update_battery_monitor_multipliers();


same as above

dpkoch

This is really, really good work; thank you @BillThePlatypus. Some suggestions of course, but this is great!

boards/airbourne/airbourne_board.cpp

src/comm_manager.cpp

dpkoch · 2020-01-09T21:37:54Z

src/config_manager.cpp

+  bool success = true;
+  for(device_t device{static_cast<device_t>(0)}; device < Configuration::DEVICE_COUNT; ++device)
+    success = success &&RF_.board_.enable_device(device, config_.config[device], RF_.params_);
+  return success;


It looks like we have an opportunity here to catch configuration errors and not allow the flight controller to arm if they exist. This could probably happen in a future pull request, as it would take some thinking and refactoring. But we could have the configuration manager report error and error resolutions to the state manager to control arming, like what we do with RC, etc.

src/comm_manager.cpp

dpkoch · 2020-01-09T21:45:26Z

src/config_manager.cpp

+uint32_t ConfigManager::generate_checksum()
+{
+  //8 bit fletcher algorithm, because we can't assume that the struct is a multiple of 16 bits
+  const uint8_t *config_data = reinterpret_cast<const uint8_t*>(config_.config);
+  uint8_t check_a{0};
+  uint8_t check_b{0};
+  for(size_t index{0}; index< sizeof(config_.config); index++ )
+  {
+    check_a += config_data[index];
+    check_b += check_a;
+  }
+  return check_a & (check_b<<8) & (~check_a<<16) & (~check_b<<24);
+}


I think this is not a true Fletcher checksum, because it's missing the modulo 255 step on check_a and check_b. You'll also want to make sure that check_a and check_b don't overflow if you're doing an actual Fletcher checksum, which means they should probably be uint16_t, and there should either be a period application of the modulo or a static_assert that sizeof(config_.config) isn't big enough to cause an overflow.

Fixing this up could probably be left to a separate PR request though, because we should probably transition the parameters and backup memory to use a Fletcher checksum as well, which should all share a common implementation.

dpkoch

Nice documentation! Just a typo or two

dpkoch · 2020-01-09T21:52:07Z

docs/user-guide/firmware-configuration.md

@@ -0,0 +1,104 @@
+# Firmware Configuration
+
+The most recent versions of ROSflight allow you to specify the hardware setup of your aircraft in greater detail. These settings are dependant on your choice of flight controller. ROSflight does not support this feature on the Naze/Flip32.


"dependant" -> "dependent"

docs/user-guide/parameter-configuration.md

BillThePlatypus · 2020-01-13T20:50:35Z

I've implemented requested changes, except for changes to the battery monitor, and changes that weren't practical, to which I responded.

superjax · 2020-02-11T03:17:35Z

What is the status of this PR?

…d mag

BillThePlatypus · 2020-03-26T20:44:09Z

This change depends on the corresponding mavlink and airbourne PR's, so those need to be merged first.

…evice_manager

# Conflicts: # .astylerc # boards/airbourne/airbourne_board.cpp # boards/breezy/breezy_board.cpp # comms/mavlink/mavlink.cpp # comms/mavlink/mavlink.h # include/board.h # include/comm_manager.h # include/command_manager.h # include/controller.h # include/estimator.h # include/interface/comm_link.h # include/mixer.h # include/param.h # include/rc.h # include/rosflight.h # include/util.h # src/comm_manager.cpp # src/command_manager.cpp # src/controller.cpp # src/mixer.cpp # src/param.cpp # src/sensors.cpp # test/command_manager_test.cpp # test/estimator_test.cpp # test/test_board.cpp # test/test_board.h # test/turbotrig_test.cpp

boards/airbourne/airbourne_board_config_manager.cpp

superjax · 2020-03-27T18:10:35Z

boards/airbourne/airbourne_board_config_manager.cpp

+  ConfigManager::ConfigResponse resp;
+  resp.reboot_required = false;
+  resp.successful = false;
+  resp.message[0] = 0; // Just in case, because a string without a null terminator causes problems


could we move this into the constructor of ConfigManager::ConfigResponse so that it's impossible to forget this?

superjax · 2020-03-27T18:12:28Z

boards/airbourne/airbourne_board_config_manager.cpp

+  }
+  resp.successful = true;
+  resp.reboot_required = true;
+  resp.message[0]=0; // Ensuring that the message is treated as a zero-length string


if we moved this to the constructor, then we won't need these lines.

superjax · 2020-03-27T18:18:06Z

docs/developer-guide/code-architecture.md

@@ -86,12 +86,21 @@ The operation of the state manager is defined by the following finite state mach

 The state manager also includes functionality for recovering from hard faults. In the case of a hard fault, the firmware writes a small amount of data to backup memory then reboots. This backup memory location is checked and then cleared after every reboot. The backup memory includes the armed state of the flight controller. On reboot, the firmware will initialize then, if this armed-state flag is set, immediately transition back into the armed state. This functionality allows for continued RC control in the case of a hard fault. Hard faults are not expected with the stable firmware code base, but this feature adds an additional layer of safety if experimental changes are being made to the firmware itself.

+### Config Manager
+This module handles the configurations for various devices, such as sensors and the serial connection. Each configuration is stored as an integer. Configurations can be set from the companion computer over the serial connection. On startup, the config manager sends configurations to the board support layer to initialize devices.


I don't know that the fact that configuration being stored as an integer important to a user.

superjax · 2020-06-20T21:58:40Z

Hey @BillThePlatypus,

I think I'm struggling to follow the Github review process.

Could you please "Resolve Conversation" if you're responded to our feedback? If all the conversations are resolved, then I think I'm happy to approve.

bsutherland333 · 2023-10-16T19:07:54Z

This looks like a lot of great work that we should revisit at some point.

BillThePlatypus mentioned this pull request Dec 7, 2019

Hardware Configuration Manager rosflight/rosflight_ros_pkgs#115

Open

superjax previously approved these changes Jan 9, 2020

View reviewed changes

BillThePlatypus dismissed superjax’s stale review via 9d417d2 January 9, 2020 21:43

dpkoch requested changes Jan 9, 2020

View reviewed changes

dpkoch reviewed Jan 9, 2020

View reviewed changes

BillThePlatypus changed the title ~~[WIP] Hardware Configuration Manager~~ Hardware Configuration Manager Jan 23, 2020

BillThePlatypus force-pushed the device_manager branch from 4bbf59b to e6a9d58 Compare March 16, 2020 22:24

BillThePlatypus added 20 commits March 16, 2020 17:07

Basic setup for device management

4a25193

Rename DeviceManager to ConfigManager

10ce42d

Define config struct

4cf3023

Basic memory manager functionality

d79af1d

Add memory manager

1f6e21f

Config manager working for serial and airspeed

4f30231

Basic functionality working for config_manager

ffefe1a

Add support for more devices in config_manager

fba096c

Finish battery monitor initialization via config manager

52cb102

Add enums for baro and mag

3833594

Add BoardConfigManager and AirbourneBoardConfigManager classes

bea06d9

Transmitting device and config names on request

d4b22ea

Add error messages to AirbourneBoardConfigManager

465e2f6

Add error messages to AirbourneBoardConfigManager

525334b

Working responses to setting configs

95a69db

Add namespace for device enum. Fix some configuration checking

22c8ca8

Code cleanup

d31b3d3

Start documentation for configurations

f8bda6b

add documentation for configurations

97ffe78

Add implementations for breezy board and test board to pass tests

b5f82bb

Fix switch fallthrough

7fb1809

BillThePlatypus force-pushed the device_manager branch from e6a9d58 to 7fb1809 Compare March 16, 2020 23:21

BillThePlatypus added 8 commits March 18, 2020 13:39

prevent disabled sensors from appearing

5a519c4

Explicitly initialize some fields

69340fe

Fix buffer overflow, add documentation to prevent, abbreviate baro an…

2b8bd6e

…d mag

Fix indentation

57892ee

Update airbourne to avoid sending baro/mag before initialization

aa60907

Cleanup, as requested in review

4e097aa

Merge branch 'master' into device_manager

5ae179b

Send number of devices

1e07c86

BillThePlatypus added 10 commits March 26, 2020 16:19

Update to latest mavlink

2ed27e0

Merge branch 'device_manager' of github.com:rosflight/firmware into d…

e8095f4

…evice_manager

Add documentation on config_list

8da5300

Additional checks for invalid configurations

1a38535

Run clang-format

b162016

More clang-foramt

99bd20b

Fix merge error

7e35fca

Fix merge breaking F1 build

6229327

Fix formatting

0f31dda

superjax reviewed May 1, 2020

View reviewed changes

BillThePlatypus added 5 commits May 5, 2020 14:33

Merge with master

18c8698

Reenable GNSS in the config manager

9862a82

Fix compilation error

9055612

fix merge error

1b3cf82

Fix style

7b5931d

fix compilation error

7d79c0d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardware Configuration Manager #369

Hardware Configuration Manager #369

BillThePlatypus commented Dec 7, 2019

BillThePlatypus commented Dec 7, 2019

superjax left a comment

superjax Jan 9, 2020

BillThePlatypus Jan 9, 2020

dpkoch Jan 9, 2020

BillThePlatypus Jan 10, 2020

superjax Jan 9, 2020

superjax Jan 9, 2020

BillThePlatypus Jan 9, 2020

superjax Jan 9, 2020

dpkoch left a comment

dpkoch Jan 9, 2020

dpkoch Jan 9, 2020

dpkoch left a comment

dpkoch Jan 9, 2020

BillThePlatypus commented Jan 13, 2020

superjax commented Feb 11, 2020

BillThePlatypus commented Mar 26, 2020

superjax Mar 27, 2020

superjax Mar 27, 2020

superjax Mar 27, 2020

superjax commented Jun 20, 2020

bsutherland333 commented Oct 16, 2023

		@@ -0,0 +1,104 @@
		# Firmware Configuration

		The most recent versions of ROSflight allow you to specify the hardware setup of your aircraft in greater detail. These settings are dependant on your choice of flight controller. ROSflight does not support this feature on the Naze/Flip32.

Hardware Configuration Manager #369

Are you sure you want to change the base?

Hardware Configuration Manager #369

Conversation

BillThePlatypus commented Dec 7, 2019

BillThePlatypus commented Dec 7, 2019

superjax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpkoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpkoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillThePlatypus commented Jan 13, 2020

superjax commented Feb 11, 2020

BillThePlatypus commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superjax commented Jun 20, 2020

bsutherland333 commented Oct 16, 2023