search

indilib / indi

INDI Core Library Repository
https://www.indilib.org
GNU Lesser General Public License v2.1
377 stars 394 forks source link

qhy ccd: Setting readout mode clears CCD Frame settings #2146

Closed munkacsymj closed 3 weeks ago

munkacsymj commented 3 weeks ago

Using indi_qhy_ccd driver and a custom client. Set CCD frame to something other than full frame using the CCD_FRAME property. Then set the chip readout mode using the READ_MODE property. Make an exposure and instead of getting the frame you set with the CCD_FRAME property, you get a full-frame exposure.

This happens whether the read mode property value is being changed or just being sent with the same (current) read mode. Either way, I don't think the intent is for the frame extent to be reset as part of setting readout mode.

Source code handling the NewNumber callback (in qhy_ccd.cpp) when READ_MODE is changed checks to see if the new read mode is different from what is was previously. There are two calls to setupParams(), one if the read mode is staying the same and one if the read mode is changing. Either way, SetupParams() will grab the chip size and call setCCDParams(), which will reset the frame to full size. I don't understand why setupParams() is ever needed when receiving a read mode setting, since I can't find anything that a read mode change would affect that would require a call to setupParams. In particular, read mode doesn't change the effective chip area, so resetting the frame seems unnecessary.

Version 2.1.0-7 Camera: QHY268M Running Linux/Ubuntu

knro commented 3 weeks ago

@nrackwit The code in question was submitted in https://github.com/indilib/indi-3rdparty/pull/341

Any comments on the above since I'm not sure if setupParams() is indeed warranted here?

nrackwit commented 3 weeks ago

Hey folks,

IMHO setupParams, or something equivalent, is indeed needed after a read mode change. Some QHYCCDs (i.e. QHY294M) in fact do change their resolution and effective area based on the read mode so in those cases the chip geometry related driver parameters all have to be re-initialized (and consequently also gain, offset and exposure times ...). I personally don't much like the fact that the code keeps re-querying the camera to initialize those variables, but that's another topic.

I think the assumption that a client would allow to change read modes on the fly without affecting other things sounds rather camera specific. It's probably gonna work for the 268M and maybe some other models so I'm not gonna claim it couldn't be done. But I sure would see how this could add a lot of camera-specific complexity to the driver. To me it seems more intuitive and straight forward to require the user to first have to pencil in the read mode and from there choose all the other settings.

Whether that second call to setupParams is really needed I'm not sure. Looking at the code it seems that this portion of the if-branch should never be reached.

Clear skies Niels

munkacsymj commented 3 weeks ago

Thank you, everyone. I had no idea that there were cameras in which read mode changes result in such dramatic behavior changes. I will just go and change the order in which properties are set.