feat: Allow different RawData types #526

toseko · 2025-10-13T07:11:25Z

No description provided.

backends/LogicalNameMapping/src/LNMBitRangeAccessPlugin.cc

src/FixedPointConverter.cc

include/FixedPointConverter.h

killenb · 2025-10-16T11:07:51Z

include/FixedPointConverter.h

+        if(_isSigned) {
+          return toRaw(std::stoi(cookedValue));
+        }
+        return toRaw(static_cast<uint32_t>(std::stoul(cookedValue))); // on some compilers, long might be a


This one has to static cast to RawType. No, wait. This cast makes no sense at all.

I think this code is broken. Needs further discussion what it does and why. What it does definitely do wong: Input string "12.7" should result in a raw value of 13. And negative numbers also don't work. I have no good implementation right now.

Possible changes for now: Use RawType or don't cast at all.

I think is should not be cast at all. I think casting was done due to fact that initially the raw type was uint32_t, but it still does not make sense as toRaw takes UserType.
Moreover, I wanted whether this casting was doing anything. It is recursive call to method toRaw(UserType) so it always cast to UserType ? or the UserType is not defined explicitly and then this automatic template type detection kicks in and type of parameter is used?

include/FixedPointConverter.h

killenb · 2025-10-16T12:33:58Z

include/FixedPointConverter.h

+    boost::fusion::for_each(_minCookedValues, initCoefficients(this));
+  }

+  using DEPRECATED_FIXEDPOINT_DEFAULT = int64_t;


Nopw, that would be int32_t

Yes, of course you are right. To keep stuff as they are 32bit should be used.

killenb

maybe we should make a static assert for the moment that the FixedPointConverter is only instantiated with int32_t?

Apart from that I think we are ready to merge to the master. I would do a full final review af the remaining issues are fixed (only did the delta of the comments and the commit, and lost the overview)

killenb · 2025-11-05T13:08:28Z

backends/LogicalNameMapping/src/LNMBitRangeAccessPlugin.cc

        // For proper implementation of this, the fixed point converter needs to signalize
        // that it had clamped. See https://redmine.msktools.desy.de/issues/12912
-        auto raw = fixedPointConverter.toRaw(buffer_2D[0][0]);
+        auto raw = (uint32_t)fixedPointConverter.toRaw(buffer_2D[0][0]);


Coding style: Don't use C-style casts.

killenb · 2025-11-05T13:15:48Z

backends/LogicalNameMapping/src/LNMBitRangeAccessPlugin.cc

      }

-      auto value = fixedPointConverter.toRaw(buffer_2D[0][0]);
+      auto value = (uint32_t)fixedPointConverter.toRaw(buffer_2D[0][0]);


killenb · 2025-11-05T13:31:07Z

include/FixedPointConverter.h

  RawType FixedPointConverter<RawType>::toRaw(UserType cookedValue) const {
    if constexpr(std::is_same<UserType, std::string>::value) {
-      if(_fractionalBits == 0) { // use integer conversion
+      // use integer conversion


Putting the comment above the if-statement makes is less precise. It belongs into the positive if branch.

killenb · 2025-11-05T13:32:03Z

include/FixedPointConverter.h

+        // different type than int...
+        //


By not having the comment behind the code that it affects makes it less clear where it belong.

killenb · 2025-11-05T13:34:17Z

include/FixedPointConverter.h

+        // on some compilers, long might be a
+        // different type than int...
+        //
+        return toRaw(std::stoul(cookedValue));


Actually, the comment refers to the cast that you removed. The comment is now wrong, and the code not robust any more to compilers where stoul does not return the RawType

killenb · 2025-11-05T15:36:52Z

include/FixedPointConverter.h

-          using converter_signed = boost::numeric::converter<int32_t, double,
-              boost::numeric::conversion_traits<int32_t, double>, boost::numeric::def_overflow_handler, Round<double>>;
-          raw = converter_signed::convert(d_cooked);
+          if constexpr(sizeof(RawType) == 4) {


I don't like the if/then/else here any more. There are too may possibilities. Especially, this does not cover 8 bit and 16 bit. We can leave it for now as an "extension" for 64 bits, but we need something better in the long run.

Yes, it becames compicated. In general adjusting to 64 bits is also complex. The FixpointConverter implementation uses heavily boost::numeric::converter. All converting from raw to coocked is also based on it. And to do it right we need to distinquish between widths of raw data. Different stuff in needed for 16, 32 and 64 bits.

killenb · 2025-11-05T15:54:10Z

include/FixedPointConverter.h

+      catch(boost::numeric::negative_overflow& e) {
+        raw = _minRawValue;
      }


For this step we leave the conversion block as it is now. It is sufficient for 32 and 64 bit conversion. We will do the improvement in ticket #13011.

What we need here now is the additional catch block for boost::numeric::positive_overflow

killenb · 2025-11-05T16:00:46Z

include/FixedPointConverter.h

-    // some sanity checks
-    if(nBits > 32) {
+                             // some sanity checks
+    const auto maxBitsNo = (sizeof(RawType) * 8);


coding style: Variable name should just be "maxBits".

No is the opposite of Yes ;-)

Yep. Probably its not correct, but I used is as abbreviation from Number - No . It's rather for enumeration so ordering, not for couting the amount of bits.

killenb · 2025-11-10T13:04:45Z

include/FixedPointConverter.h

          fpc.padUnusedBits(rawValue);
-          return numericToUserType<UserType>(*(reinterpret_cast<uint32_t*>(&rawValue)));
+          if constexpr(sizeof(RawType) == 4) {
+            return numericToUserType<UserType>(*(std::bit_cast<uint32_t*>(&rawValue)));


std::bit_cast works on an object, not on a pointer (in contrast to the reinterpret_cast), at least according to the docu. I don't know if this does the right thing. Probably it does, because it changes one pointer type into another.

At least it's unnecessarily complicated.

numericToUserType<UserType>(std::bit_cast<uint32_t>(rawValue));

should do the trick.

killenb · 2025-11-10T13:53:55Z

include/FixedPointConverter.h

+          if constexpr(sizeof(RawType) == 4) {
+            return numericToUserType<UserType>(*(std::bit_cast<uint32_t*>(&rawValue)));
+          }
+          else if constexpr(sizeof(RawType) == 8) {


I am wondering if we should separate the 64 bit case into another commit.

commit: Formally introduce RawType as template parameter, but have an assertion in the constructor that only allows int32_t as type.

commit: Test extension to 64 bits, then make it work.

We first get commit 1. ready and put it into the master before we are messing with too may things.

killenb · 2025-11-10T14:35:04Z

include/FixedPointConverter.h

+            return numericToUserType<UserType>(f * *(std::bit_cast<uint32_t*>(&rawValue)));
+          }
+          else if constexpr(sizeof(RawType) == 8) {
+            return numericToUserType<UserType>(f * *(std::bit_cast<uint64_t*>(&rawValue)));


These will start giving trouble if we have 64 bit values with more than 48 significant bits and negative fractional bits > -(64-nSignificant). I guess it's not a realistic scenario, but you would want 64 bit integer user type, which can hold the full resolution, but it is converted through double, which can only hold 48 significant bits.\

killenb · 2025-11-10T15:38:28Z

tests/executables_src/testFixedPointConverter.cpp

 #define SIGNED_HEX_TO_DOUBLE(INPUT) static_cast<double>(static_cast<int32_t>(INPUT))
 #define SIGNED_HEX_TO_INT64(INPUT) static_cast<int64_t>(static_cast<int32_t>(INPUT))

+#define SIGNED_HEX16_TO_DOUBLE(INPUT) static_cast<double>(static_cast<int16_t>(INPUT))


What about SIGNED_HEX64_TOxxx ?

killenb · 2025-11-10T15:41:21Z

tests/executables_src/testFixedPointConverter.cpp

  checkToCooked(converter, 0x40000000, 0.25);
  checkToCooked(converter, 0xC0000000, -0.25);


Why not have error messages here?

…he same fractional bit as size of the converter

feat: Allow different RawData types

9bb41af

toseko requested a review from killenb October 13, 2025 07:11

killenb reviewed Oct 16, 2025

View reviewed changes

fixup! feat: Allow different RawData types

6e0ff18

killenb reviewed Nov 5, 2025

View reviewed changes

fixup! fixup! feat: Allow different RawData types

c566c84

toseko force-pushed the feat/tkozak-allow-different-rawdata-types branch from a6c8cbf to c566c84 Compare November 6, 2025 16:30

fix: Add test for 32 bit raw and fix for it.

5ed031b

killenb reviewed Nov 11, 2025

View reviewed changes

feat: wip on testing the problem with convetion from -1 for signed, t…

5932365

…he same fractional bit as size of the converter

		checkToCooked(converter, 0x40000000, 0.25);
		checkToCooked(converter, 0xC0000000, -0.25);

feat: Allow different RawData types #526

Are you sure you want to change the base?

feat: Allow different RawData types #526

Uh oh!

Conversation

toseko commented Oct 13, 2025

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!

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants