Feature/publish encoder data #334

PilzES · 2022-08-16T11:05:48Z

Description

These changes Fix # | allow the user to | improve | ...

PR Checklist

Things to add, update or check by the maintainers before this PR can be merged.

Review Checklist

Soft- and hardware architecture (diagrams and description)
Test review (test plan and individual test cases)
Documentation describes purpose of file(s) and responsibilities
Code (coding rules, style guide)

Release planning (please answer)

When is the new feature released?
Which dependent packages do have to be released simultaneously?

Hardware tests

Unstrike the text below to enable automatic hardware tests if available. Otherwise use this as a request for the reviewer.

~~Perform hardware tests~~

martiniil

Review part 1 (28/39 files viewed)...

include/psen_scan_v2/encoder_state_ros_conversion.h

include/psen_scan_v2/ros_scanner_node.h

launch/psen_scan_v2.launch

martiniil · 2022-09-28T11:25:36Z

standalone/src/data_conversion_layer/monitoring_frame_deserialization.cpp

+        uint16_t encoder_1_read_buffer;
+        raw_processing::read<uint16_t>(ss, encoder_1_read_buffer);
+
+        uint16_t encoder_2_read_buffer;
+        raw_processing::read<uint16_t>(ss, encoder_2_read_buffer);
+
+        encoder::EncoderData encoder_data;
+        encoder_data.encoder_1 = toEncoder(encoder_1_read_buffer);
+        encoder_data.encoder_2 = toEncoder(encoder_2_read_buffer);


Consider moving those lines to a small function deserializeEncoderData(), analoguous to IOs/diagnostics.

I just read in the protocol that the encoder values are sent in reversed byte order (big endian). If I am correct then we have to further convert the values (swap the bytes like e.g. https://stackoverflow.com/questions/3823921/convert-big-endian-to-little-endian-when-reading-from-a-binary-file/3824338#3824338)

Many thanks for the great example, I believe I hit it. Check it carefully and let me know your opinion. See 842f074

standalone/src/data_conversion_layer/start_request_serialization.cpp

standalone/src/encoder_state.cpp

standalone/test/include/psen_scan_v2_standalone/util/integrationtest_helper.h

martiniil · 2022-09-28T14:54:38Z

standalone/test/integration_tests/api/integrationtest_scanner_api.cpp

@@ -148,6 +149,7 @@ ScannerConfiguration ScannerAPITests::generateScannerConfig(const std::string& h
      .scanRange(DEFAULT_SCAN_RANGE)
      .scanResolution(DEFAULT_SCAN_RESOLUTION)
      .enableIntensities()
+      .enableEncoder(ENCODER_ENABLED)


You need to include the encoder states in the ScanDataEqual matcher in matcher_and_actions.h else they are ignored by the integrationtest_scanner_api tests.

Solved. Code modified to add the encoder states at ScanDataEqual. See commit 55257b0

martiniil

Review part 2

I hope you do not get overwhelmed by my comments. Most of them are minor things.

Currently one test is failing: MonitoringFrameDeserializationTest.shouldDeserializeMonitoringFrameCorrectly. In order to make it work we have to extend WithIntensitiesAndDiagnostics in udp_frame_dumps.h with encoder data.
We should also enable the encoders in the test crcWithIntensities in unittest_start_request.cpp, where we need then to find out the new expected_crc via wireshark.

...one/include/psen_scan_v2_standalone/data_conversion_layer/monitoring_frame_deserialization.h

martiniil · 2022-09-29T09:14:29Z

standalone/src/data_conversion_layer/monitoring_frame_deserialization.cpp

+        uint16_t encoder_1_read_buffer;
+        raw_processing::read<uint16_t>(ss, encoder_1_read_buffer);
+
+        uint16_t encoder_2_read_buffer;
+        raw_processing::read<uint16_t>(ss, encoder_2_read_buffer);
+
+        encoder::EncoderData encoder_data;
+        encoder_data.encoder_1 = toEncoder(encoder_1_read_buffer);
+        encoder_data.encoder_2 = toEncoder(encoder_2_read_buffer);


I just read in the protocol that the encoder values are sent in reversed byte order (big endian). If I am correct then we have to further convert the values (swap the bytes like e.g. https://stackoverflow.com/questions/3823921/convert-big-endian-to-little-endian-when-reading-from-a-binary-file/3824338#3824338)

martiniil · 2022-09-29T09:15:06Z

standalone/test/src/data_conversion_layer/monitoring_frame_serialization.cpp

+    uint16_t encoder_1_payload = static_cast<uint16_t>(msg.encoderData().encoder_1);
+    raw_processing::write(os, encoder_1_payload);
+
+    uint16_t encoder_2_payload = static_cast<uint16_t>(msg.encoderData().encoder_2);
+    raw_processing::write(os, encoder_2_payload);


Above comment about the byte order also applies here.

Same as #334 (comment). See 842f074

standalone/include/psen_scan_v2_standalone/encoder_state.h

standalone/test/unit_tests/api/unittest_laserscan.cpp

standalone/test/unit_tests/data_conversion_layer/unittest_monitoring_frame_msg.cpp

standalone/test/unit_tests/data_conversion_layer/unittest_monitoring_frame_msg_stamped.cpp

martiniil · 2022-09-29T09:50:41Z

test/unit_tests/unittest_encoder_state_rosconversions.cpp

+
+  EXPECT_NO_THROW(psen_scan_v2::EncoderState ros_message = toEncoderStateMsg(encoder_state, "some_frame"));
+}
+


Please add tests for:

the ros_message header is set correctly

the encoder_* fields are set correctly

an exception is thrown if the supplied time is negative

The requested three tests added

test/integration_tests/integrationtest_ros_scanner_node.cpp

I agree. Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

…er/encoder_data.h Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

…on.cpp Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

…er/monitoring_frame_deserialization.h Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

martiniil · 2022-11-24T06:07:10Z

standalone/include/psen_scan_v2_standalone/data_conversion_layer/raw_processing.h

+inline void endswap(T *objp)
+{
+  unsigned char *memp = reinterpret_cast<unsigned char*>(objp);
+  std::reverse(memp, memp + sizeof(T));
+}


Using a reference instead of a pointer would align better with the rest of the code. Of course you need to change the invocations, too. E.g. raw_processing::endswap(&encoder_1_read_buffer) gets raw_processing::endswap(encoder_1_read_buffer)

We don't abbreviate the names that much, but that is only a suggestion

Suggested change

inline void endswap(T *objp)

{

unsigned char *memp = reinterpret_cast<unsigned char*>(objp);

std::reverse(memp, memp + sizeof(T));

}

inline void endianSwap(T& data)

{

unsigned char *dataPtr = reinterpret_cast<unsigned char*>(&data);

std::reverse(dataPtr, dataPtr + sizeof(T));

}

Agree totally, modified and commited in 6f7cfba

m-sanchez-rico added 4 commits August 5, 2022 14:58

first version for encoder data

d834ba8

refactor in the encoder reading feature

3247915

touch up some files

71fb0aa

Initial version of tests made for the encoder development

66fc87d

martiniil reviewed Sep 28, 2022

View reviewed changes

martiniil requested changes Sep 29, 2022

View reviewed changes

PilzES and others added 20 commits November 11, 2022 11:37

Update include/psen_scan_v2/encoder_state_ros_conversion.h

5efbf6d

I agree. Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Sure

555ce76

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

include header missed

e3b44b9

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update launch/psen_scan_v2.launch

160e44b

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update include/psen_scan_v2/ros_scanner_node.h

c151387

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update msg/EncoderState.msg

7ec7090

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update src/psen_scan_driver.cpp

30b1aca

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update standalone/CMakeLists.txt

546c19d

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update standalone/include/psen_scan_v2_standalone/data_conversion_lay…

d2b9620

…er/encoder_data.h Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update standalone/include/psen_scan_v2_standalone/data_conversion_lay…

8b88538

…er/encoder_data.h Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update standalone/include/psen_scan_v2_standalone/laserscan.h

5a11f5c

Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update standalone/src/data_conversion_layer/start_request_serializati…

629b26b

…on.cpp Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

Update standalone/include/psen_scan_v2_standalone/data_conversion_lay…

35dbfcb

…er/monitoring_frame_deserialization.h Co-authored-by: Immanuel Martini <35950815+martiniil@users.noreply.github.com>

update integrationtest_ros_scanner_node.cpp

df73987

Solve minor things of feature encoder data

6c993ef

Add new unit tests for encoder state rosconversions.

073181e

Add encoder states at ScanDataEqual.

55257b0

Small format change

fa4d3ef

Solve invalid case style.

f64b77e

Byte order review, swap between Big-Indian and Little-Indian.

842f074

martiniil requested changes Nov 24, 2022

View reviewed changes

m-sanchez-rico added 3 commits November 29, 2022 08:43

Aling code and rename function for endianSwap

6f7cfba

Fix error on standalone tests

897db60

Modify the UDP dumps to fix serialize/deserialize tests

a3f5e57

PilzES marked this pull request as ready for review December 7, 2022 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/publish encoder data #334

Feature/publish encoder data #334

PilzES commented Aug 16, 2022 •

edited

Loading

martiniil left a comment

martiniil Sep 28, 2022

martiniil Sep 29, 2022

PilzES Nov 15, 2022 •

edited

Loading

martiniil Sep 28, 2022

PilzES Nov 15, 2022

martiniil left a comment

martiniil Sep 29, 2022

martiniil Sep 29, 2022

PilzES Nov 15, 2022

martiniil Sep 29, 2022

PilzES Nov 14, 2022

martiniil Nov 24, 2022

m-sanchez-rico Nov 29, 2022


		EXPECT_NO_THROW(psen_scan_v2::EncoderState ros_message = toEncoderStateMsg(encoder_state, "some_frame"));
		}

Feature/publish encoder data #334

Are you sure you want to change the base?

Feature/publish encoder data #334

Conversation

PilzES commented Aug 16, 2022 • edited Loading

Description

Things to add, update or check by the maintainers before this PR can be merged.

Review Checklist

Release planning (please answer)

Hardware tests

martiniil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PilzES Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martiniil 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

PilzES commented Aug 16, 2022 •

edited

Loading

PilzES Nov 15, 2022 •

edited

Loading