SNS-Gyroscope #82

HTTYDKing · 2023-02-02T18:54:52Z

Separate branch for the gyroscope sensor.

… the read still need to be done

…e axis

lib/sensors/gyroscope.cpp

lib/sensors/gyroscope.hpp

…een done

lib/sensors/gyroscope.hpp

ishmis

you're getting the hang of this

lib/sensors/gyroscope.hpp

lib/sensors/gyroscope.cpp

lib/sensors/gyroscope.hpp

lib/sensors/gyroscope.cpp

…th a change of parameters

lib/core/types.hpp

lib/sensors/gyroscope.cpp

lib/sensors/gyroscope.hpp

lib/sensors/gyroscope.cpp

TomLonergan03 · 2023-02-04T21:57:19Z

lib/sensors/gyroscope.hpp

+static constexpr std::uint8_t kConfigurationSetting1   = 0xff;
+static constexpr std::uint8_t kConfigurationSetting2   = 0x20;
+static constexpr std::uint8_t kConfigurationSetting3   = 0xff;
+static constexpr std::uint8_t kConfigurationSetting5   = 0x40;


comment what these configuration values are doing

also remove the static since it isn't actually helping much with const values

maxguy2001 · 2023-02-07T13:15:05Z

lib/sensors/gyroscope.cpp

+      const auto gyroscope_x_high_byte = i2c_.readByte(kDefaultGyroscopeAddress, kDataXHigh);
+      if (!gyroscope_x_high_byte) {
+        logger_.log(
+          core::LogLevel::kFatal, "Failed to read gyroscope X-axis high at channel %d", channel_);


I thought the gyroscope only mattered if the active suspension was being used? Therefore would it not make more sense to log fatal only if active suspension is being used and just logging kWarn if active suspension is disabled @SnickeyX @kshxtij ??

This just doesn't seem like it would always be worth failing the run over...

thats an amazing catch, going to push for my overhaul to get merged to we can merge master and get kWarn here

maxguy2001 · 2023-02-07T13:18:20Z

lib/sensors/gyroscope.hpp

+static constexpr std::uint8_t kCtrl2                   = 0x21;
+static constexpr std::uint8_t kCtrl3                   = 0x22;
+static constexpr std::uint8_t kCtrl5                   = 0x24;
+static constexpr std::uint8_t kConfigurationSetting1   = 0xff;


I know we usually use full names when in doubt but kConfigurationSetting1 seems quite verbose. Could we shorten this to kConfigSetting1 since don't think this harms readability as it is a common abbreviation?

lib/sensors/gyroscope.hpp

…r the i2c pointer

lib/debug/repl.cpp

lib/sensors/gyroscope.cpp

TomLonergan03 · 2023-02-09T23:56:12Z

lib/sensors/gyroscope.hpp

+                                         = kDefaultGyroscopeAddress);
+  ~Gyroscope();
+
+  const std::optional<std::int16_t> read(core::Axis axis);


is it normal to declare return type constant? I haven't seen it elsewhere in the repo

this actually makes sense since this just enforeces that member variables arent modifed; although the const should be suffixed

TomLonergan03 · 2023-02-10T22:36:41Z

lib/sensors/gyroscope.cpp

+      logger_.log(core::LogLevel::kFatal, "Gave an invalid axis");
+      return std::nullopt;
+    }
+  }


i think this should probably be restructured to have a readAxis helper function and maybe pass in kDataXHigh and kDataXLow as parameters, right now this is a lot of almost duplicate code.

lib/sensors/gyroscope.cpp

TomLonergan03 · 2023-02-10T22:42:18Z

lib/sensors/gyroscope.hpp

+                                         std::shared_ptr<io::II2c> i2c,
+                                         const std::uint8_t channel,
+                                         const std::uint8_t device_address
+                                         = kDefaultGyroscopeAddress);


i am not keen on having default parameters, if gyroscopes will always have the address kDefaultGyroscopeAddress then rename to kGyroscopeAddress and drop the device_address parameter and instead use the constant, or if a gyro might have other addresses then the address should always be passed in when constructing.

it can have two different addresses, @HTTYDKing make sure you add the "alternative address" as well, think its 0x68

maxguy2001 · 2023-02-16T18:37:41Z

lib/debug/repl.cpp

+    std::uint16_t axis;
+    std::cout << "axis (0 is x, 1 is y, 2 is z): ";
+    std::cin >> axis;
+    std::optional<std::int16_t> value;


maxguy2001 · 2023-02-16T18:42:59Z

lib/sensors/gyroscope.hpp

+
+class Gyroscope {
+ public:
+  static std::optional<Gyroscope> create(core::ILogger &logger,


Do we still explicitly call static? @SnickeyX

yes here its necessary, else create will not be able to access gyroscope specifc functions

maxguy2001 · 2023-02-16T18:45:36Z

lib/sensors/gyroscope.cpp

+{
+}
+
+const std::optional<std::int16_t> Gyroscope::read(core::Axis axis)


Does const not go after instead of before?

maxguy2001 · 2023-02-16T18:46:33Z

lib/sensors/gyroscope.cpp

+      const auto x_axis = ((*gyroscope_x_high_byte << 8) | *gyroscope_x_low_byte);
+      logger_.log(
+        core::LogLevel::kDebug, "Successfully read x-axis from gyroscope at channel %d", channel_);
+      return static_cast<std::uint16_t>(x_axis * 0.00875);


What is 0.00875 and where did it come from?

Also elsewhere in the file. Maybe give it a name and store it somewhere?

ishmis · 2023-02-16T22:45:04Z

@HTTYDKing remember to merge master for merge conflicts

maxguy2001 · 2023-04-06T14:35:24Z

lib/core/types.hpp

@@ -62,6 +62,18 @@ struct RawAccelerationData {
  const TimePoint measured_at;
 };

+struct GyroscopeData {


What's the use for this? Is there no more suitable alternatives?

Shouldn't this be a static thing that can get passed somewhere for full processing?

maxguy2001 · 2023-04-06T14:39:22Z

lib/debug/repl.cpp

+                  "Missing required field 'sensors.gyroscope.bus' in configuration file");
+      return std::nullopt;
+    }
+    const auto device_address = gyroscope["device_address"].GetUint();


Did you check this exists first?

maxguy2001 · 2023-04-06T14:41:12Z

lib/debug/repl.cpp

+    auto Old_time = std::chrono::system_clock::now();
+    std::optional<core::GyroscopeData> value;
+
+    while (true) {


In general, we try to avoid making loops like this. Although not exactly adhered to, we try to follow NASA's rules for safety critical code, one of which (specifically rule 2) is that all loops must have a statically defined upper bound to avoid infinite loops and other such upsetting happenings.

maxguy2001 · 2023-04-06T14:41:46Z

lib/debug/repl.cpp

+      std::optional<core::GyroscopeData> value = gyroscope->read(core::Axis::kX);
+      const auto result = value.value();
+
+      std::chrono::duration<double> elapsed_seconds = result.measured_at - Old_time;


Something like previous_timestamp?

maxguy2001 · 2023-04-06T14:42:16Z

lib/debug/repl.cpp

+
+      std::chrono::duration<double> elapsed_seconds = result.measured_at - Old_time;
+
+      auto time = (core::Float) elapsed_seconds.count();


static_cast<core::Float>(...)

also, more informative naming?

maxguy2001 · 2023-04-06T14:44:10Z

lib/debug/repl.cpp

+      Old_time = result.measured_at;
+      Old_data = result.x;
+
+      logger_.log(core::LogLevel::kInfo, "Gyroscope is : %f", Angle);


"Gyroscope is "? This doesn't make much sense?

maxguy2001 · 2023-04-06T14:45:38Z

lib/debug/repl.hpp

@@ -14,6 +14,9 @@
 #include <io/hardware_uart.hpp>
 #include <io/pwm.hpp>
 #include <sensors/accelerometer.hpp>
+#include <sensors/gyroscope.hpp>
+#include <unistd.h>


For consistency, think we use cstdint here instead?

maxguy2001 · 2023-04-06T14:45:59Z

lib/debug/repl.hpp

@@ -14,6 +14,9 @@
 #include <io/hardware_uart.hpp>
 #include <io/pwm.hpp>
 #include <sensors/accelerometer.hpp>
+#include <sensors/gyroscope.hpp>
+#include <unistd.h>
+#include <chrono>


Is this used?

maxguy2001 · 2023-04-06T14:47:41Z

lib/sensors/gyroscope.cpp

+    = i2c->writeByteToRegister(device_address, kCtrl1, kConfigurationSetting1);
+  if (write_result_from_ctrl1 == core::Result::kFailure) {
+    logger.log(
+      core::LogLevel::kFatal,


Again, isn't this only critical failure if we're actually using active suspension?

maxguy2001 · 2023-04-06T14:52:11Z

lib/debug/repl.cpp

+      std::optional<core::GyroscopeData> value = gyroscope->read(core::Axis::kX);
+      const auto result = value.value();
+
+      std::chrono::duration<double> elapsed_seconds = result.measured_at - Old_time;


Have a look in core. Most of that functionality should already be implemented and if possible, we want to keep the ugly chrono notation contained in there.

HTTYDKing added 3 commits January 26, 2023 19:03

Step up

20d1e51

This is the first version of the Gyroscope sensor, the conversions of…

895097c

… the read still need to be done

First version of the Gyroscope code, need to add the conversion of th…

3cb7643

…e axis

HTTYDKing requested a review from ishmis as a code owner February 2, 2023 18:54

maxguy2001 requested changes Feb 2, 2023

View reviewed changes

lib/sensors/gyroscope.cpp Outdated Show resolved Hide resolved

lib/sensors/gyroscope.hpp Outdated Show resolved Hide resolved

The changes for the logger and the naming convention for kData have b…

5f7fbc9

…een done

maxguy2001 requested changes Feb 3, 2023

View reviewed changes

lib/sensors/gyroscope.hpp Outdated Show resolved Hide resolved

ishmis requested changes Feb 3, 2023

View reviewed changes

lib/sensors/gyroscope.hpp Outdated Show resolved Hide resolved

lib/sensors/gyroscope.cpp Outdated Show resolved Hide resolved

lib/sensors/gyroscope.cpp Outdated Show resolved Hide resolved

lib/sensors/gyroscope.hpp Outdated Show resolved Hide resolved

maxguy2001 requested changes Feb 4, 2023

View reviewed changes

lib/sensors/gyroscope.cpp Outdated Show resolved Hide resolved

not fully done trying to solve error

c246add

HTTYDKing requested a review from kshxtij as a code owner February 4, 2023 13:49

HTTYDKing added 6 commits February 4, 2023 13:57

Added the required parameters for read and the new create funtion

99e4927

Added the required parameters for read and the new create funtion

5244163

Added the required parameters for read and the new create function wi…

1ebe8d9

…th a change of parameters

removing redundant hyped::

f3c35e8

Added the required parameters for read and the new create function wi…

568db5f

…th a change of parameters

Added the required parameters for read and the new create function wi…

c163bc6

…th a change of parameters

HTTYDKing requested review from maxguy2001 and ishmis February 4, 2023 14:16

ishmis requested changes Feb 4, 2023

View reviewed changes

Changes to formating

b2a0e6e

maxguy2001 requested changes Feb 4, 2023

View reviewed changes

lib/sensors/gyroscope.cpp Outdated Show resolved Hide resolved

lib/sensors/gyroscope.cpp Outdated Show resolved Hide resolved

HTTYDKing requested review from maxguy2001 and ishmis February 4, 2023 16:09

HTTYDKing added 2 commits February 4, 2023 16:40

Changing the if statements in read to switch

24ba279

adding const to getChannel

e4f1a3b

TomLonergan03 requested changes Feb 4, 2023

View reviewed changes

Merge branch 'master' into sns-gyro

ff664bd

maxguy2001 requested changes Feb 7, 2023

View reviewed changes

Adding the repl commands or gyroscope and making changes to adjust fo…

99c6aba

…r the i2c pointer

TomLonergan03 requested changes Feb 9, 2023

View reviewed changes

Changes made to the formating

ed1aa15

HTTYDKing requested review from TomLonergan03 and maxguy2001 February 10, 2023 20:17

TomLonergan03 requested changes Feb 10, 2023

View reviewed changes

maxguy2001 requested changes Feb 16, 2023

View reviewed changes

Relevant changes with struct but issue still prosides

91d7bcc

maxguy2001 requested changes Apr 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNS-Gyroscope #82

SNS-Gyroscope #82

HTTYDKing commented Feb 2, 2023

ishmis left a comment

TomLonergan03 Feb 4, 2023

ishmis Feb 5, 2023

maxguy2001 Feb 7, 2023

maxguy2001 Feb 7, 2023

ishmis Feb 11, 2023

maxguy2001 Feb 7, 2023

TomLonergan03 Feb 9, 2023

ishmis Feb 11, 2023

TomLonergan03 Feb 10, 2023

TomLonergan03 Feb 10, 2023 •

edited

Loading

ishmis Feb 11, 2023

maxguy2001 Feb 16, 2023

maxguy2001 Feb 16, 2023

ishmis Feb 16, 2023

maxguy2001 Feb 16, 2023

maxguy2001 Feb 16, 2023

maxguy2001 Feb 16, 2023

ishmis commented Feb 16, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023

maxguy2001 Apr 6, 2023


		std::chrono::duration<double> elapsed_seconds = result.measured_at - Old_time;

		auto time = (core::Float) elapsed_seconds.count();

SNS-Gyroscope #82

Are you sure you want to change the base?

SNS-Gyroscope #82

Conversation

HTTYDKing commented Feb 2, 2023

ishmis 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

Choose a reason for hiding this comment

TomLonergan03 Feb 10, 2023 • edited Loading

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

ishmis commented Feb 16, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomLonergan03 Feb 10, 2023 •

edited

Loading