Adding the example for the sensor based data collection and running on a riscv device by shrit · Pull Request #250 · mlpack/examples

shrit · 2026-06-28T13:30:11Z

No description provided.

Signed-off-by: Omar Shrit <omar@avontech.fr>

github-actions · 2026-06-28T13:30:19Z

👈 Launch a binder notebook on branch shrit/examples/mpu

rcurtin

I am really hesitant about this design. The amount of complexity in this code is huge: it contains its own I2C library specific to each of the sensors in the example, and I think if I understand right that it is basically written entirely by Claude?

(It also seems like the training and inference programs aren't here, but I assume you will push them soon.)

The question I have is: who is the target audience here? Nobody wants to read 3000 lines of hardware control code to understand it. How will someone approach the example? The other examples we have are all written so that someone can read through the entire source and know what is going on. But that's unrealistic here. Do you think we can go through and pare back the code a huge amount? It's an example, not an enterprise application, so all of the "fancy" stuff like short-option and long-option parsing are just unnecessary, in my opinion.

There is a huge amount of other error checking and support code that's also probably unnecessary (at least that is what I have seen with lots of other Claude-generated code). When there is too much code, even if it really is checking for every possible case, a reader will never make it through the whole file and learn what we want them to learn (i.e. how to use mlpack). In my eyes that makes it sort of defeat the purpose of the tutorial.

rcurtin · 2026-07-01T16:05:36Z

@@ -0,0 +1,725 @@
+# - Find mlpack


How will we keep this file up to date? There are now copies of mlpack.cmake floating all over the place, but the upstream one in mlpack updates from time to time.

This is a very good question, that I do not have the answer for it, maybe using the CI ? github actions that can check every new PR ?

Signed-off-by: Omar Shrit <omar@avontech.fr>

… inference as well Signed-off-by: Omar Shrit <omar@avontech.fr>

Signed-off-by: Omar Shrit <omar@avontech.fr>

Signed-off-by: Omar Shrit <omar@shrit.me>

… quickly Signed-off-by: Omar Shrit <omar@shrit.me>

Signed-off-by: Omar Shrit <omar@shrit.me>

shrit · 2026-10-15T13:20:34Z

@rcurtin I think this one is ready and it should be easier to review, I have also tested the code for the three program in addition to the IMU testing one and they all looks good, feel free to give it a review when you have a chance

…hardware Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin

I know I have a lot of comments, but I hope they are useful. I really had to work hard to not comment on every awful verb choice by Claude; Claude really likes to personify programs in ways that are inappropriate and obscure the technical meaning of what's actually going on. But... below driver/ I really tried to not care, since users won't really be reading there and will focus on train.cpp/infer.cpp/collect.cpp and the CMake configuration.

rcurtin · 2026-11-06T16:04:58Z


 CXX := g++
-CXXFLAGS += -std=c++17 -Wall -Wextra -O3 -DNDEBUG -fopenmp
+CXXFLAGS += -std=c++17 -Wall -Wextra -DMLPACK_ENABLE_HTTPLIB -O3 -DNDEBUG -fopenmp


After https://github.com/mlpack/mlpack/pull/4201/changes is merged, then httplib is enabled by default, so I think we can remove this from here and other Makefiles?

rcurtin · 2026-11-06T16:13:55Z

+#         -DOPENBLAS_PATCHES=../CMake/patches/openblas-riscv64-low-memory.patch ..
+#   make
+#
+# Build a single program with, e.g., `make collect` or `make train`.


Could be useful to give a URL to the tutorial here too, in case this is the first place a user looks. Up to you.

rcurtin · 2026-11-06T17:22:55Z

+ * mlpack is free software; you may redistribute it and/or modify it under the
+ * terms of the 3-clause BSD license.  You should have received a copy of the
+ * 3-clause BSD license along with mlpack.  If not, see
+ * http://www.opensource.org/licenses/BSD-3-Clause for more information.


The tutorial no longer discusses this, so I think it would be okay to either remove or simplify it (or I mean if you want to keep it as-is that's fine too). Since the example doesn't actually use the magnetometer, maybe all the calibration code is unneeded. But I haven't checked in-depth.

rcurtin · 2026-11-06T17:27:40Z

+
+IMU::IMU(const I2CBus& bus, uint8_t gyroAddress, uint8_t accelMagAddress) :
+    gyro(bus, gyroAddress),
+    accelMag(bus, accelMagAddress),


What about the calibration of the magnetometer? If the code no longer uses calibration at all, then we should remove mag_calibration.hpp and all references to it (including in imu_test.cpp).

rcurtin · 2026-11-06T17:28:18Z

+{
+  next += period;
+  std::this_thread::sleep_until(next);
+}


This is a really unhelpful utility function here. If this file gets kept this should definitely be inlined.

rcurtin · 2026-11-06T17:30:06Z

+  static constexpr uint8_t kWhoAmIValue = 0xD7;
+  //! Some clone GY-89 boards carry the older L3GD20 (non-H), which is
+  //! register-compatible but reports this WHO_AM_I instead.
+  static constexpr uint8_t kWhoAmIValueL3GD20 = 0xD4;


The Hungarian notation kVariableName is a bit weird to me because it isn't used anywhere else. But this is a minor comment (because this code is very deep in the sensor controller code).

rcurtin

I know I have a lot of comments, but I hope they are useful. I really had to work hard to not comment on every awful verb choice by Claude; Claude really likes to personify programs in ways that are inappropriate and obscure the technical meaning of what's actually going on. But... below driver/ I really tried to not care, since users won't really be reading there and will focus on train.cpp/infer.cpp/collect.cpp and the CMake configuration.

Co-authored-by: Ryan Curtin <ryan@ratml.org>

shrit added 8 commits June 20, 2026 15:27

Add the drivers for the GY89 MPU and barometer BMP 180

08a1422

Signed-off-by: Omar Shrit <omar@avontech.fr>

Adding a Makefile for the collect dataset project, use ncurses

cdffaa3

Signed-off-by: Omar Shrit <omar@avontech.fr>

Adding the collect binary that collect data w/o TUI

15e633a

Signed-off-by: Omar Shrit <omar@avontech.fr>

Adding the CMake configuration that is needed for cross compilation

cdf960b

Signed-off-by: Omar Shrit <omar@avontech.fr>

Adding necessary CMakeLists file needed for the cross compilation

9c3a920

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix capture the next arg, and also make TUI by default unless otherwise

711fa01

Signed-off-by: Omar Shrit <omar@avontech.fr>

Make ncurses compiles by default unless the user say otherwise

434635d

Signed-off-by: Omar Shrit <omar@avontech.fr>

Adding OpenBLAS patch in CMakelist before the compilation

39ed301

Signed-off-by: Omar Shrit <omar@avontech.fr>

rcurtin reviewed Jul 1, 2026

View reviewed changes

shrit and others added 20 commits July 13, 2026 19:55

Remove makefiles as pre review

26d1747

Signed-off-by: Omar Shrit <omar@avontech.fr>

Remove completely ncurses

0c792dd

Signed-off-by: Omar Shrit <omar@avontech.fr>

Simplify the code largely to make it easier to read

6278731

Signed-off-by: Omar Shrit <omar@avontech.fr>

Move manually the sensor code into its own class

a2160d7

Signed-off-by: Omar Shrit <omar@avontech.fr>

Inline mag calibration class

05fb789

Signed-off-by: Omar Shrit <omar@avontech.fr>

Remove the cpp from the list of files to compile

84d8402

Signed-off-by: Omar Shrit <omar@avontech.fr>

Write openblas function that apply the low memory patch modification

32544aa

Signed-off-by: Omar Shrit <omar@avontech.fr>

Move the parse sensor to sensor_board class since it is being used by…

be89ac9

… inference as well Signed-off-by: Omar Shrit <omar@avontech.fr>

Simplify the argc argv input according to rcurtin comment

0d61be2

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix the architecture flag, and match our mlpack modifications

bef370a

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix the author name

0a8753f

Signed-off-by: Omar Shrit <omar@avontech.fr>

Train function added, first version

ce3e999

Signed-off-by: Omar Shrit <omar@avontech.fr>

Adding comments relevant to the matrix shape changes

5252fef

Signed-off-by: Omar Shrit <omar@avontech.fr>

Fix the window size in the comments to be 256 which is 2.5 seconds

4a7eaa3

Signed-off-by: Omar Shrit <omar@shrit.me>

Add the patch for openblas matrix multiplication buffer size

052d59d

Signed-off-by: Omar Shrit <omar@shrit.me>

Add a local README only for this project, to make it easy to evaluate…

d524d8e

… quickly Signed-off-by: Omar Shrit <omar@shrit.me>

Add a first version for the inference loop

d4b793c

Signed-off-by: Omar Shrit <omar@shrit.me>

Change the name from collect_main to collect to match others

e71f44e

Signed-off-by: Omar Shrit <omar@shrit.me>

Get rid of cstdio, and use fstream and iostream

f648c6c

Signed-off-by: Omar Shrit <omar@shrit.me>

Make the model saved inside its own model directory

0451e98

Signed-off-by: Omar Shrit <omar@shrit.me>

Use model directory in both training and inference

aba9757

Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin reviewed Aug 22, 2026

View reviewed changes

Comment thread cpp/movement_recognition/CMakeLists.txt Resolved

rcurtin reviewed Aug 22, 2026

View reviewed changes

Comment thread cpp/movement_recognition/CMake/ConfigureCrossCompile.cmake Resolved

rcurtin reviewed Aug 22, 2026

View reviewed changes

Comment thread cpp/movement_recognition/CMake/ConfigureCrossCompile.cmake Resolved

shrit added 14 commits September 23, 2026 16:08

Simplify the code for the input and make the window size fixed

48471f2

Signed-off-by: Omar Shrit <omar@shrit.me>

Fix the comments, simplify the code

87cddc7

Signed-off-by: Omar Shrit <omar@shrit.me>

Remove magnometer calibration

0ed972e

Signed-off-by: Omar Shrit <omar@shrit.me>

Remove mangometer calibration, since it is not relevant to the project

d1d40c8

Signed-off-by: Omar Shrit <omar@shrit.me>

Fix the load function call, do not use deprecated function

7b638fc

Signed-off-by: Omar Shrit <omar@shrit.me>

Do not use deprecated functions

28c810c

Signed-off-by: Omar Shrit <omar@shrit.me>

Use new API with DataOptions and httplib

737b23b

Signed-off-by: Omar Shrit <omar@shrit.me>

Apply the new Scaler template class, no need to convert fmat and mat

35c7c48

Signed-off-by: Omar Shrit <omar@shrit.me>

Update CMake with the one from the master version

e775d37

Signed-off-by: Omar Shrit <omar@shrit.me>

Simplify the CmakeLists file

33b1357

Signed-off-by: Omar Shrit <omar@shrit.me>

Add OpenBLAS flag

28bfe5d

Signed-off-by: Omar Shrit <omar@shrit.me>

Fix the load function

f4323cf

Signed-off-by: Omar Shrit <omar@shrit.me>

Serialize all the models in one class, it makes the code easier

e4c2033

Signed-off-by: Omar Shrit <omar@shrit.me>

Update CmakeFile and cross compile config in the entire directory

cc48678

Signed-off-by: Omar Shrit <omar@shrit.me>

Fix the output because of different address for different version of …

2ada441

…hardware Signed-off-by: Omar Shrit <omar@shrit.me>

rcurtin reviewed Nov 6, 2026

View reviewed changes

shrit and others added 8 commits November 8, 2026 16:30

Update cpp/movement_recognition/README.md

572674d

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update cpp/lstm/stock_prediction/lstm_stock_prediction.cpp

11161e4

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update cpp/movement_recognition/driver/sensor_board.hpp

ce15b1c

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update cpp/lstm/electricity_consumption/lstm_electricity_consumption.cpp

98d12ea

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update cpp/movement_recognition/model.hpp

de0ba2a

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update cpp/movement_recognition/model.hpp

d4a6741

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update cpp/movement_recognition/train/train.cpp

20879c5

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Update cpp/movement_recognition/train/train.cpp

f21f1f5

Co-authored-by: Ryan Curtin <ryan@ratml.org>

Conversation

shrit commented Jun 28, 2026

Uh oh!

github-actions Bot commented Jun 28, 2026

Uh oh!

rcurtin left a comment

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!

shrit commented Oct 15, 2026

Uh oh!

rcurtin left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rcurtin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants