logger - using ModuleParams

[BazookaJoe1900]

and some cleanups on main()

Describe problem solved by this pull request
using ModuleParams in logger module.

@dagar can you confirm/fixme if while using ModuleParams should I call ModuleParams::updateParams() somewhere?

[bkueng]

Thanks.
All of the params require a reboot, so you don't need to check for changes. But it would be good to add it anyway to avoid future surprises. You need to subscribe to parameter_update and call updateParams(); if it changes.

[bkueng]

This will not be correct if -f -e is specified. This is why the logic is as it is now.

[BazookaJoe1900]

@bkueng, thanks again. but I think you wrong here. this is exactly what I wanted to fix here, the confusion.
as default log_on_start = false; log_until_shutdown = false;

-e set log_on_start = true;
-f set log_on_start = true; log_until_shutdown = true;

later:
if (log_on_start) {
if (log_until_shutdown) { log_mode = Logger::LogMode::boot_until_shutdown;
} else { log_mode = Logger::LogMode::boot_until_disarm;}}

any chance you ment if -f -e are both specified? that can't be

[bkueng]

Yes I meant if both are specified, which is valid and correct (-f implies -e).

[BazookaJoe1900]

@bkueng - to be clear, there is no fix required here? its all good, and more clear now, right?

[bkueng]

No, it's incorrect now. Please revert back to how it was.

[BazookaJoe1900]

@bkueng It took me some time to understand you. I see now what you meant. My thought is that its either using -e or -f or -x. and not some kind of combinations.
on the previous algorithm using '-f' or '-f -e' or '-e -f' did the exact some thing.
On my "way" you should use only one of them.
yes, it breaks something on current confusing use.
is there way to enforce it, you must call with one of the argument only?
And I don't talk about how -x confuse the current logic....

[bkueng]

From a CLI perspective this is not confusing, it's clearly documented that -f imples -e.
Only in code there's a bit more logic to handle this. I don't see a problem with this.
Changing the CLI is an option, but then you must return an error if both are provided - the end result won't really be simpler.

[BazookaJoe1900]

@bkueng - I can't say that I am 100% agree, but I have changed to logic according to your recommendation on this commit

[BazookaJoe1900]

After some rebasing issue... Ready to re-review

[BazookaJoe1900]

@bkueng

All of the params require a reboot, so you don't need to check for changes. But it would be good to add it anyway to avoid future surprises. You need to subscribe to parameter_update and call updateParams(); if it changes.

that done, can you review?