Add C recording interface by dacuster · Pull Request #146 · LORD-MicroStrain/mip_sdk

dacuster · 2025-08-14T16:43:11Z

Add C recording interface
Improved all connection interfaces
Simplified device interface
Added a few missing C++ wrappers

Added C recording connection Refactored all connections in C++ to extend C Refactored device interface to simplify usage and added connection parameters Added timestamping functions

Temporarily removed broken examples until they are merged

Forgot to disable the broken C examples

Simplified recording interface initialization

Fixed formatting and naming conventions for consistency

microstrain-sam

The Serial and TCP drivers shouldn't depend on the recording connection. They're not intended to be "connections" at all - just basic drivers with read/write functionality. This keeps them modular and reusable. E.g. a customer might have a system with serial ports but no file system (including stdio at all, even if it's not used, can be a problem for embedded systems).

If we want a more "connection"-oriented approach, that should be done on top of the basic drivers, just like we do in C++. This way, a customer can provide their own backend driver (serial or whatever else) and still use the recording/connection layers on top. The serial port (and every single other connection driver) shouldn't have to implement anything to do with recording. serial_port_close_receive_recording_stream should just be recording_stream_close_receive or similar. Also, the way it's writtne, how could one close the recording stream without knowing the connection type at compile time?

There are a lot of formatting changes which should be in their own PR. Especially files where the code isn't any different (e.g. platform_connection.cpp). In some places I can't see the real code changes because the diff tool gets confused (mip_interface.hpp).

All that said, I really like having the connection pointer in the mip_interface (this is required by the firmware updater actually, currently it assumes the user pointer is the connection), and we definitely need better recording support. I think this is a big enough subject that we should have a group discussion about it before committing to a particular approach.

microstrain-sam · 2025-09-05T15:33:11Z

src/c/microstrain/embedded_time.h

+////////////////////////////////////////////////////////////////////////////////
+/// @brief Gets the current system timestamp in milliseconds
+///
+/// @details Provides basic timestamping using system time:
+///          - Returns milliseconds since Unix epoch
+///          - Uses timespec_get() with UTC time base
+///          - Returns 0 if time cannot be obtained
+///
+/// @return Current system time in milliseconds since epoch
+///
+static inline microstrain_embedded_timestamp microstrain_get_current_timestamp()
+{
+    struct timespec ts;
+
+    // Get system UTC time since epoch
+    if (timespec_get(&ts, TIME_UTC) != TIME_UTC)
+    {
+        return 0;
+    }
+
+    // Get the time in milliseconds
+    return (microstrain_embedded_timestamp)ts.tv_sec * 1000 + (microstrain_embedded_timestamp)ts.tv_nsec / 1000000;
+}
+


This is only legal on desktop systems. I 100% agree this function should be in the mip sdk, but it needs to go under extras or connections or similar, where it's only compiled for the systems that have it available. Similar for send_to_device, receive_from_device, etc.

How is this only legal on desktop? It's part of the C standard

The C standard has two parts, the core language and the runtime library. Generally speaking, every platform supports the core language. But the library is a different story, and many platforms only have partial support. For example, in our products, where would timespec_get get the time? The only concept of time is the one we've defined, i.e. reference time and external/GPS time. The C library would have no way of knowing about them. Similar issues arise for filesystem access; many embedded systems have no filesystem and thus don't support fopen/etc.

microstrain-sam · 2025-09-05T15:36:53Z

src/c/mip/definitions/common.h

+#ifdef __cplusplus
+void insert_mip_descriptor_rate(microstrain::C::microstrain_serializer *serializer, const mip_descriptor_rate *self);
+void extract_mip_descriptor_rate(microstrain::C::microstrain_serializer *serializer, mip_descriptor_rate *self);
+#else
 void insert_mip_descriptor_rate(microstrain_serializer *serializer, const mip_descriptor_rate *self);
 void extract_mip_descriptor_rate(microstrain_serializer *serializer, mip_descriptor_rate *self);
+#endif // __cplusplus


Interesting that this is suddenly a problem... I've encountered it before but I'm not sure why it went away.

This can happen in a few different places, so it might make sense to define a helper macro, MICROSTRAIN_C_NAMESPACE that could be placed before C identifiers, like this:

#ifdef __cplusplus #define MICROSTRAIN_C_NAMESPACE microstrain::C:: #else #define MICROSTRAIN_C_NAMESPACE #endif

void insert_mip_descriptor_rate(MICROSTRAIN_C_NAMESPACE microstrain_serializer* serializer, ...);

Then we wouldn't need the ifdef / else / endif on the function declarations.

We need to do away with the C namespace entirely. It's causes issues like this all over the place and everything is already in a "namespace" in C since we prepend microstrain and mip to everything

Perhaps... but the namespace does help distinguish the C api from the C++ one, especially for things like autocomplete. We also don't do a perfect job of "namespacing" in the C api (e.g. insert_* and extract_* rather than microstrain_insert_*, etc).

microstrain-sam · 2025-09-05T16:02:01Z

src/cpp/microstrain/connections/connection.hpp

+            virtual bool readSpanUpdate(Span<uint8_t>& _buffer, const uint32_t _wait_time_ms,
+                EmbeddedTimestamp& _timestamp_out) final;


virtual final makes no sense here. final means it can't be overridden, so why make it virtual at all? Just use a non-virtual function.

It doesn't make sense, but it won't compile as final without virtual. Adding this also guarantees is can't be overwritten otherwise overwritten functions just hide this instead

I wouldn't expect users to re-implement wrapper functions in derived classes, as long as we're clear about what functions are intended to be overridden (the virtual keyword implies intent for overrides). Especially inline functions, a simple wrapper would be fairly obvious what it's doing and that it wouldn't make sense to override.

It might seem to make sense to mark non-virtual functions as final because they're not meant to be overridden, but that's implied by being non-virtual. There are at least a few compilers that give warnings about "hiding" functions; if that warning is turned on, we don't need final here.

microstrain-sam · 2025-09-05T16:08:46Z

src/cpp/microstrain/connections/connection.hpp

+            virtual bool read(uint8_t* _buffer, const size_t _byte_count, const uint32_t _wait_time_ms,
+                size_t& _bytes_read_out, EmbeddedTimestamp& _timestamp_out) = 0;


We're moving away from uint8_t* buffer, size_t length towards Span<uint8_t> for buffer parameters. I recommend making the readSpan version the pure virtual function (rename to just read), and if we still want it, this version can be the wrapper. The reason for this is that the span version is easier to use and can still be called with separate buffer and length parameters, like this: connection.read({buffer, sizeof(buffer)}, wait_time, &bytes_read, &timestamp);

I completely disagree with Span and we need to get rid of it. It's not compatible with std::span and there's no need for it. It's also not a concept in C anyway. I've already run into a problem with the Span interface when I switch from std::span to microstrain::Span that I needed to fix because it's not a complete wrapper. We just shouldn't have it. I also don't remember having a meeting to discuss the use of Span over buffer/length

I was mostly referring to embedded firmware; we're moving towards span there as it tends to make the code significantly cleaner and more readable. Would you agree that print(myVector) is more readable than print(myVector.data(), myVector.size())?

However, there is precedent for moving towards span in the MIP SDK as well. First, there is metadata, which requires various kinds of span-like structures internally. Adding pointer+length metadata API functions has no benefit. Second, there was discussion about this exact issue in the strings PR that was merged recently. I was recommended to remove the pointer+length overloads of the C++ wrappers for string formatting functions. The span version is directly usable in all cases (as I described in my first comment above). The pointer+length overloads added no additional capabilities and came with the downsides of more function overloads and extra maintenance and testing.

More generally, in C++:
Span is a useful concept and it doesn't make sense to throw it away. We have C arrays, std::vector, and std::array as the standard contiguous data containers. Span is the only container-like object that can hold a view of contiguous data without owning/copying it.

Consider working with a range of elements contained in a vector. Without span, one needs to use indices, pointer/length, or begin/end iterators. This is inconsistent with how one works with vectors as a whole (e.g. range-based for loops and passing to functions). Span provides consistency by providing a "container" representing the subelements.

Structures representing a range of elements (i.e. spans) are found everywhere, e.g. PacketView. It doesn't make sense to define a new structure of (pointer+length, or begin+end) every time we need one. Just use span.

In C, there are no standard data structures, so these arguments don't apply the same way. I do still believe there are still benefits to span-like structures and algorithms (e.g. subspan) however, even if that's just a set of functions that operate on pointer+length rather than a struct.

microstrain::Span is intended to be directly interchangeable with std::span. If it's not, that's a bug and a separate issue that should be resolved. If you find such a problem again, please bring it to my attention.

microstrain-sam · 2025-09-05T16:09:23Z

src/cpp/microstrain/connections/connection.hpp

+            virtual bool readSpan(const Span<uint8_t>& _buffer, const uint32_t _waitTimeMs,
+                size_t& _bytesReadOut, EmbeddedTimestamp& _timestampOut) final;


Rename to just read and make this the main function instead of the wrapper.

No, we need to stop using Span and if we do it needs to be a utility

microstrain-sam · 2025-09-05T16:10:36Z

src/cpp/microstrain/connections/connection.cpp

+        void Connection::initializeReceiveRecordingStream(FILE* _receiveStream)
+        {
+            mRecordingConnection.initializeReceiveStream(_receiveStream);
+        }
+
+        void Connection::initializeReceiveRecordingFile(const char* _receiveFileName)
+        {
+            mRecordingConnection.openReceiveFile(_receiveFileName);
+        }
+
+        void Connection::closeReceiveRecordingStream()
+        {
+            mRecordingConnection.closeReceiveFile();
+        }
+
+        void Connection::initializeSendRecordingStream(FILE* _sendStream)
+        {
+            mRecordingConnection.initializeSendStream(_sendStream);
+        }
+
+        void Connection::initializeSendRecordingFile(const char* _sendFileName)
+        {
+            mRecordingConnection.openSendFile(_sendFileName);
+        }
+
+        void Connection::closeSendRecordingStream()
+        {
+            mRecordingConnection.closeSendFile();
+        }
+
+        void Connection::initializeRecordingStreams(FILE* _receiveStream, FILE* _sendStream)
+        {
+            mRecordingConnection.initializeStreams(_receiveStream, _sendStream);
+        }


These (and some others) can be put inline in the header. We haven't done this much with the C mip sdk, but that's something that should be changed for performance reasons.

We need to keep the headers clean of implementations for readability purposes. I'd rather do something like Dan proposed with .inl files for things like this instead

I'm ok with using .inl files.

microstrain-sam · 2025-09-05T16:11:40Z

src/cpp/microstrain/embedded_time.hpp

+    static inline EmbeddedTimestamp getCurrentTimestamp()
+    {
+        const std::chrono::nanoseconds timeSinceEpoch = std::chrono::system_clock::now().time_since_epoch();
+        return static_cast<EmbeddedTimestamp>(
+            std::chrono::duration_cast<std::chrono::milliseconds>(timeSinceEpoch).count());
+    }


Can't do this on embedded. Maybe move to platform_connection.cpp?

What part of this can't be done on embedded systems? The only thing different from what was already in the serial_connection.cpp::recvFromDevice implementation is system_clock over steady_clock

std::chrono is not be available on some embedded systems.

Using it in serial_connection.cpp is ok because it gets excluded in embedded systems where MICROSTRAIN_ENABLE_SERIAL is disabled. embedded_time.hpp is unconditionally included.

microstrain-sam · 2025-09-05T16:39:16Z

src/c/microstrain/connections/serial/serial_port.h

+    /// @brief Communication speed in bits per second
+    uint32_t baudrate;


Generally speaking, it's best to use a single source of truth for any piece of data. In this case, the true baudrate is held by the operating system, and can be accessed via system calls. Duplicating it here perhaps makes it a bit easier to read the baudrate, but risks becoming out of sync (say, if the customer accesses the handle directly, or if the handle is shared with other things). Additionally, it would feel natural to say serial_port->baudrate = 921600 but that wouldn't actually change the baudrate.

Sure, but generally speaking the user should know to use the functions provided to make proper changes to these members. I can look into those systems calls instead to see the possibility of just using that

github-actions · 2025-09-11T20:09:53Z

🚨 Changelog Check Failed

Please update CHANGELOG.md with your changes before merging.

Alternatively, include skip-changelog in ANY commit message to bypass this check for this PR.

Nick DaCosta added 9 commits August 14, 2025 12:36

Refactor of connections and device interface

8ed5708

Added C recording connection Refactored all connections in C++ to extend C Refactored device interface to simplify usage and added connection parameters Added timestamping functions

Updated examples for device interface and connection changes

327ef3f

Removed broken examples

7a1dce9

Temporarily removed broken examples until they are merged

Removed broken C examples

e300843

Forgot to disable the broken C examples

Fixed strlen missing header

ca68270

Fixed typo in documentation

846dfd3

Added recording connection to stream IMU examples

eb602ce

Updated default Linux port name

c8474ad

Added READMEs for stream IMU examples

ce8861a

dacuster marked this pull request as draft August 15, 2025 16:02

Nick DaCosta added 6 commits August 18, 2025 14:59

Fixed inconsistent naming convention for recording

42af102

Updated stream IMU examples

e9dbb98

Simplified recording interface initialization

Syntax and typo fixes

1deb7ee

Added C and C++ recording examples

7ad9d03

Syntax cleanup

a6f1c75

Fixed formatting and naming conventions for consistency

Syntax and typo fixes

b338df0

dacuster marked this pull request as ready for review August 20, 2025 13:47

dacuster force-pushed the feature/cRecordingConnections branch from 1b0aa29 to b338df0 Compare August 29, 2025 19:40

microstrain-sam suggested changes Sep 5, 2025

View reviewed changes

Nick DaCosta added 2 commits September 11, 2025 10:01

Merge branch 'develop' into feature/cRecordingConnections

c05622a

Fixed timestamping in examples

b612fef

		virtual bool readSpanUpdate(Span<uint8_t>& _buffer, const uint32_t _wait_time_ms,
		EmbeddedTimestamp& _timestamp_out) final;

		virtual bool read(uint8_t* _buffer, const size_t _byte_count, const uint32_t _wait_time_ms,
		size_t& _bytes_read_out, EmbeddedTimestamp& _timestamp_out) = 0;

		virtual bool readSpan(const Span<uint8_t>& _buffer, const uint32_t _waitTimeMs,
		size_t& _bytesReadOut, EmbeddedTimestamp& _timestampOut) final;

		/// @brief Communication speed in bits per second
		uint32_t baudrate;

Conversation

dacuster commented Aug 14, 2025

Uh oh!

microstrain-sam 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

microstrain-sam Sep 5, 2025 • 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!

github-actions bot commented Sep 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

microstrain-sam Sep 5, 2025 •

edited

Loading