Stop overwriting Rad Enable state

[Sheyne]

The firmware previously set the radiometry to ENABLE every time Y16 streaming started. The new behavior is to leave radiometry alone and to turn TLinear off when rgb888 starts and restore the TLinear state when rgb888 stops.

[Sheyne]

Thanks for taking the time to review this. If you look at what it's doing, it clears the cache when it sets the value, so it only sets the value once (when closing the rgb888 stream). Thus we don't need to worry about restoring the cache multiple times. I've verified this experimentally.

1. Turn on TLinear using the extension unit
2. Turn on rgb stream
3. Read TLinear (it is off)
4. Turn off rgb stream
5. Read TLinear (it is on)
6. Turn off TLinear using the extension unit
7. Toggle any stream (rgb, y16, etc)
8. Read TLinear (it is off, as hoped)

Per is arguing that TLinear is one of a significant number of parameters he might want to set, and that having a frame format for each combination of parameter-states would be overwhelming and hard to access.

Does this solution seem reasonable in light of the fact that it only restores the cached value once?

[kekiefer]

OK, I missed the part where this thing cleared the cache after restoring it, which meets my ask -- to push altered state only around rgb888, and do it every time. I looked this over a few of times and missed that, and I think that speaks to my point about this being a little confusing.

I agree with your instinct to keep this simple, and I fully support that.

My ask is simply to make it a little easier to read and understand, not to make this any more generic. How about this: instead of overloading the cached_TLinear_state variable with the cache validity, simply keep track of this variable separately. I know it introduces yet another global, but it should make this already confusing state machine a bit easier to grok.

[Sheyne]

I'll push an update

[Sheyne]

Should I add a field indicating what mode (y16, rgb888, etc) we're coming from too?

[kekiefer]

I don't think so, at least not now. It seems sufficient to be able to say when streaming ends "there's stuff in the cache, restore it", and until we need to make a determination based on mode which values to restore, that's probably overkill.

[Sheyne]

Pushed. Verified with the same set of steps as above. With any stream in step 7 as = y16

[kekiefer]

That's much easier to understand, thanks for that change.

Now I wonder, would this get called the first time into the loop and set the uninitialized cache value?

[Sheyne]

That's a great point! What do you think of this?

[kekiefer]

Looks good. Merging into development.

[kekiefer]

Thanks Sheyne!