HasActionsIF + CommandMessage Typo #145

muellerr · 2020-07-16T12:52:57+02:00

muellerr commented

2020-07-16 12:52:57 +02:00

HasActionsIF interface change: size_t used instead of uint32_t
Also fixed CommandMessage Typo.

The initialize() function takes queue to use as nullptr default argument now because it can be set with a separate setter function as well.

HasActionsIF interface change: size_t used instead of uint32_t Also fixed CommandMessage Typo. The initialize() function takes queue to use as nullptr default argument now because it can be set with a separate setter function as well.

muellerr added the

feature

API Change

labels 2020-07-16 12:52:57 +02:00

muellerr referenced this pull request

2020-07-16 12:53:15 +02:00

DHB Update #144

mohr self-assigned this 2020-07-21 11:23:50 +02:00

mohr commented

2020-07-21 11:23:54 +02:00

What is the usecase for being able to set the queue to use after initialized is called? The helper is intended to be used tightly coupled to one object whose queue should not change after initialize.

muellerr commented

2020-07-25 10:47:41 +02:00

DeviceHandlerBase initialize:

result = actionHelper.initialize(commandQueue);
if (result != RETURN_OK) {
	return result;
}

ActionHelper is a member class, so it has to be constructed in the initializer list.
The command queue will be created in the constructor, so a nullptr is passed to the action helper in the initializer list and the queue is assigned in the initialize function.

DeviceHandlerBase initialize: ```cpp result = actionHelper.initialize(commandQueue); if (result != RETURN_OK) { return result; } ``` ActionHelper is a member class, so it has to be constructed in the initializer list. The command queue will be created in the constructor, so a nullptr is passed to the action helper in the initializer list and the queue is assigned in the initialize function.

mohr commented

2020-08-04 11:34:20 +02:00

My question is why the queue parameter to initialize() is changed to be optional. I can not see a reason to allow not setting the queue in initialize().

muellerr commented

2020-08-04 11:43:51 +02:00

AH, because it does not need to be set if it is set in the constructor.
But it will propably always be set in initialize all the time. I can make in non-optional again.

AH, because it does not need to be set if it is set in the constructor. But it will propably always be set in `initialize` all the time. I can make in non-optional again.

mohr commented

2020-08-04 12:18:11 +02:00

Ok, now I see. I did not realize one could set it before initialize(). I am a bit torn, because it is reasonable to not force people to set it twice (have it optional in initialize()), but I also want to make it hard to forget setting it (have it non-optional in initialize()).

But I think we need to trust users at some point so I think you are right with having it optional.

Ok, now I see. I did not realize one could set it before initialize(). I am a bit torn, because it is reasonable to not force people to set it twice (have it optional in initialize()), but I also want to make it hard to forget setting it (have it non-optional in initialize()). But I think we need to trust users at some point so I think you are right with having it optional.

mohr referenced this issue from a commit

2020-08-04 12:18:29 +02:00

Merge pull request 'HasActionsIF + CommandMessage Typo' (#145) from KSat/fsfw:mueller/feature/HasActionsIF into master

mohr closed this pull request

2020-08-04 12:18:29 +02:00

mohr commented

2020-08-04 12:28:12 +02:00

I added documentation to warn users only to omit the queue in initialize() if they have set it in the constructor in commit 78442a8b92.

I added documentation to warn users only to omit the queue in initialize() if they have set it in the constructor in commit 78442a8b92b5548098fca7439a78ba65978de4b5.

petri referenced this issue from a commit

2020-09-16 07:06:50 +02:00

Amending PR #145, forgot some documentation

gaisser deleted branch mueller/feature/HasActionsIF

2020-12-08 14:19:57 +01:00

Sign in to join this conversation.

No reviewers