GetCoarseTimeOffset takes an IDs::source but we pass an IDs::channel::str()

[AndrewEdmonds11]

As mentioned in pull request #211, I have a question about calling GetCoarseTimeOffset. Here's what I wrote

At the moment, SetupNavigator::GetCoarseTimeOffset() takes an IDs::source but what I see in the code is that we give it IDs::channel::str(). If I understand correctly, this will set the generator and config of the source to any. So how does SetupNavigator know which row to get? (I will open a separate issue for discussion on this)

I guess @jrquirk could clarify

[jrquirk]

That's broken code. It should be a source, I just never tested that branch of the code before I made whatever changes rendered that bad.

It might be a good idea to declare IDs::source(std::string) explicit so the compiler can catch these mistakes, or it might not if people like the conversion.

[AndrewEdmonds11]

I think the conversion may be fine in some cases (e.g. if you don't know the exact config or generator that was used) but for this it is important.

There's a GetSource() method that is equivalent to GetChannel() in TVAnalysedPulseGenerator.h. Also in that file, the channel and generator of fSource are set in SetChannel() and the constructor respectively. Is the configuration set somewhere as well?

[jrquirk]

I meant an automatic conversion from string to source. @benkrikler would probably have an informed opinion.

And I don't know if the config is set. This can be bad because a match to any config should just return the first channel+gen-config in the list, which wouldn't get caught at compile time or run time.

Thank you for noticing this.

[benkrikler]

Since we were passing in IDs::channel::str() I don't think making the constructor explicit would have helped. We need to avoid using the str() method for this sort of task and only use it for making human readable strings or interacting with code outside of the framework (such as root objects or the STL).

[jrquirk]

I changed this to SetupNavigator::Instance()->GetCoarseTimeOffset(GetSource()) in the CFTimeMBAmp generator. But I don't think we'll be using this generator much since it's the same as FirstComplete without the integral.