SNS-Gyroscope by HTTYDKing · Pull Request #82 · Hyp-ed/hyped-2023

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

Separate branch for the gyroscope sensor.

… the read still need to be done

…e axis

…een done

ishmis

you're getting the hang of this

…th a change of parameters

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

+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

+      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

+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?

…r the i2c pointer

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

+                                         = 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

+      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.

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

+                                         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

+    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

+
+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

+{
+}
+
+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

+      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

  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

+                  "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

+    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

+      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

+
+      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

+      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

 #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

 #include <sensors/accelerometer.hpp>
+#include <sensors/gyroscope.hpp>
+#include <unistd.h>
+#include <chrono>


Is this used?

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

+    = 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

+      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

Comment thread lib/sensors/gyroscope.cpp Outdated

Comment thread lib/sensors/gyroscope.hpp Outdated

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

Comment thread lib/sensors/gyroscope.hpp Outdated

ishmis requested changes Feb 3, 2023

View reviewed changes

Comment thread lib/sensors/gyroscope.hpp Outdated

Comment thread lib/sensors/gyroscope.cpp Outdated

Comment thread lib/sensors/gyroscope.cpp Outdated

Comment thread lib/sensors/gyroscope.hpp Outdated

maxguy2001 requested changes Feb 4, 2023

View reviewed changes

Comment thread lib/sensors/gyroscope.cpp Outdated

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 ishmis and maxguy2001 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

Comment thread lib/sensors/gyroscope.cpp Outdated

Comment thread lib/sensors/gyroscope.cpp Outdated

HTTYDKing requested review from ishmis and maxguy2001 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


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

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

Uh oh!

Conversation

HTTYDKing commented Feb 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ishmis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TomLonergan03 Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishmis commented Feb 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomLonergan03 Feb 10, 2023 •

edited

Loading