add new data wrapper helper type #668

muellerr · 2022-08-30T12:08:29+02:00

muellerr commented

2022-08-30 12:08:29 +02:00

Helper class using a tagged union to wrap user data.

Right now, two formats are supported:

Classic C style const uint8_t* and size pair
SerializeIF

Helper class using a tagged union to wrap user data. Right now, two formats are supported: - Classic C style `const uint8_t*` and size pair - `SerializeIF`

muellerr added 1 commit 2022-08-30 12:08:32 +02:00

fsfw/fsfw/pipeline/head Build started... Details

fsfw/fsfw/pipeline/pr-development This commit looks good Details

20d42add03

add new data wrapper helper type

muellerr requested review from gaisser 2022-08-30 13:18:00 +02:00

muellerr added 1 commit 2022-08-30 13:25:02 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

0f27c7e7e7

extend data wrapper

muellerr added 1 commit 2022-08-30 13:39:44 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

c756297e5c

data wrapper update

muellerr added a new dependency 2022-08-30 13:41:16 +02:00

#669 DHB refactor TM handler

muellerr requested review from mohr 2022-08-30 13:44:21 +02:00

muellerr added this to the v6.0.0 milestone 2022-08-30 13:44:24 +02:00

muellerr added 1 commit 2022-08-30 14:03:08 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

bdd79d060d

basic data wrapper unittests

muellerr added 1 commit 2022-08-30 14:03:32 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

192255df1c

additional test

muellerr added the

feature

label 2022-08-30 14:03:52 +02:00

muellerr added 1 commit 2022-08-30 14:41:41 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

d675a789a2

update changelog

muellerr added 1 commit 2022-08-30 16:02:49 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

8d1777fa0c

additional tests

muellerr added 1 commit 2022-08-30 16:04:49 +02:00

fsfw/fsfw/pipeline/pr-development This commit looks good Details

fsfw/fsfw/pipeline/head This commit looks good Details

20f0707813 remove newline

mohr commented

2022-08-30 18:25:57 +02:00

As said in #669, I would think of C style const uint8_t* and size pairs as a subset of SerializeIF via SerialBufferAdapter.

As said in #669, I would think of C style `const uint8_t*` and size pairs as a subset of `SerializeIF` via `SerialBufferAdapter`.

muellerr commented

2022-08-30 19:03:22 +02:00

This is a feature I have already used it inside the CFDP stack implementation and for packaging TM replies directly inside Device Handler Base. Having used it at two places inside our code, I considered this useful enough to make it generic and create a PR for it.

I can look into using SerialBufferAdapter for those cases..

This is a feature I have already used it inside the CFDP stack implementation and for packaging TM replies directly inside Device Handler Base. Having used it at two places inside our code, I considered this useful enough to make it generic and create a PR for it. I can look into using `SerialBufferAdapter` for those cases..

muellerr commented

2022-08-30 19:27:30 +02:00

Alright, I actually created this union type to have a helper which would wrap both SerializeIF and the raw pointer / size combination.

When I see an API taking a SerializeIF, it takes implicit knowledge and requires cognitive overhead to remember that the serial buffer adapter can be used for this purpose.

In my opinion, there should be the following 2 functions available

foo(const uint8_t* data, size_t dataLen);
foo(SerializeIF* data)

when providing an API that takes user data. The first way is a very common way for C++ containers to provide initial data. It's easy to grasp for beginners as well.

The class above could be used to avoid having 2 APIs for the same purpose.. Another solution would be to always expose the 2 functions mentioned above at all places where I used this new data wrapper and forward the first variant to the second using SerialBufferAdapter.

However, considering a configuration struct like this

struct PusTmParams {
  PusTmParams() = default;
  explicit PusTmParams(PusTmSecHeader secHeader) : secHeader(secHeader){};
  PusTmParams(PusTmSecHeader secHeader, util::DataWrapper dataWrapper)
      : secHeader(secHeader), dataWrapper(dataWrapper) {}

  PusTmParams(uint8_t service, uint8_t subservice, TimeStamperIF* timeStamper)
      : secHeader(service, subservice, timeStamper) {}

  PusTmParams(uint8_t service, uint8_t subservice, TimeStamperIF* timeStamper,
              util::DataWrapper dataWrapper_)
      : PusTmParams(service, subservice, timeStamper) {
    dataWrapper = dataWrapper_;
  }
  PusTmSecHeader secHeader;
  util::DataWrapper dataWrapper{};
};

this would require two additional functions to expose this API.

Alright, I actually created this union type to have a helper which would wrap both `SerializeIF` and the raw pointer / size combination. When I see an API taking a SerializeIF, it takes implicit knowledge and requires cognitive overhead to remember that the serial buffer adapter can be used for this purpose. In my opinion, there should be the following 2 functions available ```cpp foo(const uint8_t* data, size_t dataLen); foo(SerializeIF* data) ``` when providing an API that takes user data. The first way is a very common way for C++ containers to provide initial data. It's easy to grasp for beginners as well. The class above could be used to avoid having 2 APIs for the same purpose.. Another solution would be to always expose the 2 functions mentioned above at all places where I used this new data wrapper and forward the first variant to the second using `SerialBufferAdapter`. However, considering a configuration struct like this ```cpp struct PusTmParams { PusTmParams() = default; explicit PusTmParams(PusTmSecHeader secHeader) : secHeader(secHeader){}; PusTmParams(PusTmSecHeader secHeader, util::DataWrapper dataWrapper) : secHeader(secHeader), dataWrapper(dataWrapper) {} PusTmParams(uint8_t service, uint8_t subservice, TimeStamperIF* timeStamper) : secHeader(service, subservice, timeStamper) {} PusTmParams(uint8_t service, uint8_t subservice, TimeStamperIF* timeStamper, util::DataWrapper dataWrapper_) : PusTmParams(service, subservice, timeStamper) { dataWrapper = dataWrapper_; } PusTmSecHeader secHeader; util::DataWrapper dataWrapper{}; }; ``` this would require two additional functions to expose this API.

muellerr referenced this pull request

2022-09-01 08:47:56 +02:00

Refactor TMTC Stack, improve test framework #655

gaisser added a new dependency 2022-09-01 11:48:47 +02:00

#655 Refactor TMTC Stack, improve test framework

gaisser removed a dependency 2022-09-01 11:48:57 +02:00

#655 Refactor TMTC Stack, improve test framework

gaisser added a new dependency 2022-09-01 11:49:44 +02:00

#655 Refactor TMTC Stack, improve test framework

mohr removed a dependency 2022-09-01 14:14:16 +02:00

#669 DHB refactor TM handler

muellerr removed a dependency 2022-09-05 16:02:42 +02:00