WIP: RSDK-11082: Remove instances of C-style pointer manipulation #30

lia-viam · 2025-07-25T15:42:54Z

This PR modernizes some of the older C-style pointer code in favor of modern approaches using boost::span and range-based for loops or algorithms from <algorithm>. I will annotate the PR below with comments; feel free to suggest if you think they should be put into the code as comments also

Marking as draft for now pending some further testing

lia-viam · 2025-07-25T15:44:17Z

src/module/orbbec.cpp

+        return boost::span{bytes.get(), size};
+    }

-    static constexpr deleter_type array_delete_deleter = [](unsigned char* ptr) { delete[] ptr; };


note that

it is UB to free memory that was new'd

the std::default_delete template parameter for unique_ptr<T[]> already does delete[]
https://en.cppreference.com/w/cpp/memory/default_delete.html

lia-viam · 2025-07-25T15:45:19Z

src/module/orbbec.cpp


 std::vector<unsigned char> RGBPointsToPCD(std::shared_ptr<ob::Frame> frame, float scale) {
-    int numPoints = frame->dataSize() / sizeof(OBColorPoint);
+    assert(frame->is<ob::PointsFrame>());


If this pattern comes up more often we could write a FrameView<T> class which performs the following asserts and returns a span of the data already set to type T

what kind of error message is produced when an assert fails?

It includes the line like:

orbbec.cpp:152 frame->is<ob::PointsFrame>() assert failed

So it includes the line number and string of the failing assert.

lia-viam · 2025-07-25T15:51:57Z

src/module/orbbec.cpp

-    std::memcpy(rawBuf, encodedData.data(), encodedData.size());
+    raw_camera_image rawBuf(encodedData.size());
+
+    std::copy(encodedData.begin(), encodedData.end(), rawBuf.span().begin());


note that most standard library implementations of std::copy will fall back to a compiler intrinsic version of memcpy or memmove when operating on contiguous iterators of arithmetic types

SebastianMunozP · 2025-07-25T18:18:02Z

src/module/orbbec.cpp

+    }

-    static constexpr deleter_type array_delete_deleter = [](unsigned char* ptr) { delete[] ptr; };
+    boost::span<unsigned char> span() noexcept {


Not critical at all, but could we use std::uint8_t instead of unsigned char to express bytes?

that works!

@SebastianMunozP I was looking into this and it seems like we may want to use either unsigned char or std::byte, as per these guidelines: https://stackoverflow.com/a/77097674 but they advise against std::uint8_t

I believe this project is C++17 so we could use std::byte instead of unsigned char, do you have thoughts or preferences?

huh, didn't know about std::byte :) , I agree, let's go with that then.

just added some changes using std::byte. unfortunately since the Viam SDK is still on C++14 there is no access to std::byte so we end up having to convert back to unsigned char. Please let me know your thoughts, we can keep it this way or revert to the previous diff using unsigned char

I see, in that case let's just go back to unsigned char and try this change maybe in the future.

SebastianMunozP · 2025-07-25T18:20:19Z

src/module/orbbec.cpp

+    assert(frame->is<ob::PointsFrame>());
+    assert(frame->getFormat() == OBFormat::OB_FORMAT_RGB_POINT);
+
+    auto pointSpan = boost::span{reinterpret_cast<OBColorPoint*>(frame->data()), frame->dataSize() / sizeof(OBColorPoint)};


Love this :)

how do we ensure that the span's underlying bytes do not get freed while using it?

@hexbabe that's the responsibility of the calling code; span is a non-owning reference or "view" type, meaning it provides a different view or interface upon some underlying data without taking responsibility for the lifetime of the data. Similar to string_view, or language-level references (int i; int& j = i;).

see https://stackoverflow.com/a/45723820 for further discussion

SebastianMunozP · 2025-08-01T14:41:27Z

src/module/orbbec.cpp


 std::vector<unsigned char> RGBPointsToPCD(std::shared_ptr<ob::Frame> frame, float scale) {
-    int numPoints = frame->dataSize() / sizeof(OBColorPoint);
+    assert(frame->is<ob::PointsFrame>());


I like using asserts, but in this case I would prefer to detect the case of these two asserts and log it and early return. I think it would be better to handle this by looking at logs than forcing the module to terminate.

Cc. @nicksanford in case you want to weigh in.

If the assert fails does it terminate the os process?

If it terminates the os process does Viam server get an unambiguous log that would tell us immediately why the process terminated?

Is it impossible for the assertion to ever fail in practice given the current state of the code?

If the answers to all those questions are 'yes' then this is fine.

If the answer to any of them is no then I'll need a justification for why this asset is here.

yes (via calling std::abort())

yes (we will get a message stating the file/line of the assert and the assert that failed)

I tested this module and the assertions did not failed

It's worth noting that as the code is at the moment, these asserts are being removed from the binary since this is a release build. I enabled them by creating a debug build and making sure that I could hit asserts.

Having assets fire in release builds sounds good to me.

I'd also be ok with us writing our own assert function. Whichever you feel is best.

So in the current code as is the assert wouldn't get hit unless someone called RGBPointsToPCD(not_an_rgb_point_frame). The asserts are there to show how we can check type safety given that we are dealing with polymorphic data but in this function only one type of input is acceptable.

Per another comment #30 (comment) the use of assert here was meant to sketch a general procedure that could be used if this pattern keeps coming up. Namely, we have an ob::Frame* frame, and we want to get a range which refers to a definite underlying type. In this case it would be nice to be able to just do

void f(std::shared_ptr<ob::Frame> frame){ FrameView<OBColorPoint> frameView(frame); }

and we would expect this to be an error if frame were not in fact an OB_FORMAT_RGB_POINT, similar to how some standard library containers have asserts or bounds checking in debug builds.

I think assert or throw is the right behavior here; this will fail loudly with a meaningful log message and viam-server can restart the module. An early return would result in further control flow being executed with an empty vector, which could be easy to miss.

A possible option, since we already use Boost, is Boost.Assert. This has the advantage of letting us

keep asserts in non-debug builds independent of NDEBUG

write a different assertion processing function (eg, log error, throw exception) rather than the assert behavior of calling abort

That's a good idea, let's do that, let's change the asserts for BOOST_ASSERT

hexbabe

approving, but mostly just read over and asked questions for learning

hexbabe · 2025-08-07T20:03:16Z

src/module/orbbec.cpp

+    assert(frame->is<ob::PointsFrame>());
+    assert(frame->getFormat() == OBFormat::OB_FORMAT_RGB_POINT);
+
+    auto pointSpan = boost::span{reinterpret_cast<OBColorPoint*>(frame->data()), frame->dataSize() / sizeof(OBColorPoint)};


how do we ensure that the span's underlying bytes do not get freed while using it?

hexbabe · 2025-08-07T20:03:47Z

src/module/orbbec.cpp

-    }
+    auto depthPixels = boost::span{reinterpret_cast<uint16_t const*>(data), height * width};
+
+    std::transform(depthPixels.begin(), depthPixels.end(), m.begin(), [](uint16_t mi) { return mi * mmToMeterMultiple; });


this is sick

may i recommend this for inspo 🙂
https://youtu.be/W2tWOdzgXHA?t=133

hexbabe · 2025-08-07T20:05:43Z

src/module/orbbec.cpp


 std::vector<unsigned char> RGBPointsToPCD(std::shared_ptr<ob::Frame> frame, float scale) {
-    int numPoints = frame->dataSize() / sizeof(OBColorPoint);
+    assert(frame->is<ob::PointsFrame>());


what kind of error message is produced when an assert fails?

hexbabe · 2025-08-07T20:06:44Z

Actually I'll hold off approval until manual tests are reported to ensure no regressions

hexbabe

blocked on test writeup

lia-viam added 3 commits July 24, 2025 13:54

rewrite raw camera image and rgb to pcd

6e59f48

condense helper invocation

c7e807c

use span

c0c7fe6

lia-viam requested review from SebastianMunozP and hexbabe July 25, 2025 15:43

lia-viam commented Jul 25, 2025

View reviewed changes

SebastianMunozP reviewed Jul 25, 2025

View reviewed changes

Merge branch 'main' into ptr-convert

4aa2f81

lia-viam force-pushed the ptr-convert branch from e62deb5 to 4aa2f81 Compare July 30, 2025 13:31

SebastianMunozP reviewed Aug 1, 2025

View reviewed changes

hexbabe approved these changes Aug 7, 2025

View reviewed changes

hexbabe self-requested a review August 7, 2025 20:06

hexbabe requested changes Aug 7, 2025

View reviewed changes

WIP: RSDK-11082: Remove instances of C-style pointer manipulation #30

Are you sure you want to change the base?

WIP: RSDK-11082: Remove instances of C-style pointer manipulation #30

Uh oh!

Conversation

lia-viam commented Jul 25, 2025

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!

lia-viam Aug 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!

hexbabe 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!

hexbabe commented Aug 7, 2025

Uh oh!

hexbabe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lia-viam Aug 5, 2025 •

edited

Loading