OpenPHDGuiding / phd2

PHD2 Guiding
https://openphdguiding.org
BSD 3-Clause "New" or "Revised" License
255 stars 115 forks source link

QHY native driver improvements #1245

Closed daleghent closed 4 days ago

daleghent commented 1 month ago

After some exploratory work, I'm re-titling this issue to be more relevant.

The QHY native driver can use some enhancement to address 1 problem and bring it in line with modern QHY SDK feature sets.

  1. Explicitly call SetQHYCCDBitsMode() during Connect() to set the camera output to either 8 or 16 bit. This would help correct an issue with the saturation level camera configuration item where, in some cases, a saturation of 255 would be enforced despite the camera outputting 16bit data.
  2. Add support for QHY cameras with coolers.
  3. Add support for specifying an offset. Currently, only the gain is controlled and there is no way to specify an offset. This can result in many pixels becoming clipped, and more will be clipped as gain value increases.
  4. Add support for "amplifier noise reduction" feature present on some QHY camera models. This is done by manipulating the CONTROL_AMPV camera control.
  5. Add support for QHY5II-M row noise reduction option
  6. Add support for QHY5III178 / QHY178 high gain mode
  7. Optimize exposure+data readout timing based on feedback from Dr. Q @ QHY
daleghent commented 1 month ago

WIP branch here https://github.com/daleghent/phd2/tree/qhy-improvements

daleghent commented 1 month ago

WIP camera options window

image

bwdev01 commented 1 month ago

Hi Dale, this looks good at this point. My question is whether we understand the peculiar behavior reported regarding the bits-per-pixel property. In particular, I want to know why we’re constraining the MaxADU value to be 255. That property is represented by a text field in the UI so the constraining mechanism should be coming from this code when the form is closed:

saturationVal = wxMin(val, SaturationValFromBPP(m_pCamera));

This suggests that a read of the bits-per-pixel value is returning 8 even though the camera is returning 16-bit data. I didn’t see anything in your code that did anything in this area although perhaps I missed it. AFAICT, the m_bpp variable in cam_qhy.cpp is being set correctly after the camera is connected so I don’t understand what’s wrong – and of course, this has worked fine for all the previous QHY cameras. It may be worth remembering that we’re apparently dealing with a beta version of the camera driver here.

From: Dale Ghent @. Sent: Wednesday, October 16, 2024 10:59 PM To: OpenPHDGuiding/phd2 @.> Cc: Subscribed @.***> Subject: Re: [OpenPHDGuiding/phd2] QHY native driver improvements (Issue #1245)

WIP camera options window

image.png (view on web) https://github.com/user-attachments/assets/389934d9-1bab-446e-9a64-5698f6912661

— Reply to this email directly, view it on GitHub https://github.com/OpenPHDGuiding/phd2/issues/1245#issuecomment-2418567342 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ADDHSV7BLEPHV4I4U2T7BGLZ35GZHAVCNFSM6AAAAABPV4U5Y6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJYGU3DOMZUGI . You are receiving this because you are subscribed to this thread. https://github.com/notifications/beacon/ADDHSV4DNZVX7I7GPI33I4LZ35GZHA5CNFSM6AAAAABPV4U5Y6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUQFBUK4.gif Message ID: @. @.> >

daleghent commented 1 month ago

Reviewing their logs, the user was connecting and reconnecting with multiple cameras from QHY and ZWO, with both their respective ASCOM and native drivers. In short, the log contents were all over the place, as there were multiple profiles being switched to. This is what I've been able to distill:

  1. Connected to QHY5III462M via QHY ASCOM driver; previous camera was "ZWO ASI290MM Mini" via native ZWO driver
    • BitsPerPixel=8
    • FullSize=(0,0)
  2. Reconnected to the same, same image dimensions/bpp reported
  3. Connected to QHY native driver which reported BPP=16. User kept SaturationADU at 255.

So, the QHY ASCOM driver, via the PHD2 ASCOM client, kept asserting a BPP of 8. Curious of where this came from, I went into the PHD2 ASCOM client code and found that it derives the SaturationADU parameter using the driver's MaxADU property. Since I have a QHY5III462C on my desk, I connected to it in PHD2 via the QHY ASCOM driver and also got a BPP of 8.

I fired up the ASCOM Diagnostics app and used that to connect to a it via the QHY ASCOM driver. Annoyingly, ASCOM Diagnostics doesn't dump all camera driver properties so I could not see what the driver was reporting for MaxADU, but it does dump the contents of the driver's ASCOM profile storage. To my surprise, there was a MaxADU variable associated with the camera model/serial number stored in the profile with a value of 255:

QHY5III462C-5764394f9f37839b2ElectronsPerADU           1
QHY5III462C-5764394f9f37839b2FullWellCapacity          65535
QHY5III462C-5764394f9f37839b2Gain                      100
QHY5III462C-5764394f9f37839b2HasCooler                 False
QHY5III462C-5764394f9f37839b2HasShutter                False
QHY5III462C-5764394f9f37839b2MaxADU                    255
QHY5III462C-5764394f9f37839b2MaxBinX                   2
QHY5III462C-5764394f9f37839b2MaxBinY                   2

I'm guessing that the QHY ASCOM driver uses the ASCOM Profile system as a per-camera key:value store, and all these camera parameters aren't dynamically derived but are actually presets! So I fired up the ASCOM Profile Explorer and found it:

image

So I edited the QHY5III462C-5764394f9f37839b2MaxADU field to have a value of 65535. Reconnecting to the QHY ASCOM driver in PHD2, it now reports BPP=16:

21:32:14.070 00.001 210664 Connected Camera: QHY5III462C-5764394f9f37839b2 (ASCOM)
21:32:14.071 00.001 210664 FullSize=(0,0)
21:32:14.071 00.000 210664 PixelSize=2.90
21:32:14.072 00.001 210664 BitsPerPixel=16
21:32:14.072 00.000 210664 HasGainControl=0
21:32:14.072 00.000 210664 HasShutter=0
21:32:14.073 00.001 210664 HasSubFrames=1
21:32:14.073 00.000 210664 ST4HasGuideOutput=1

So it seems that the QHY ASCOM driver is hard-coded to report a MaxADU of 255 and only by editing the camera's entry in the driver's ASCOM profile storage area can you induce it to report a MaxADU of 65535. Ha. So that explains the source of the 8bit saturation level.

The user's other complaint was that the native driver worked but it operating in 8bit mode. The logs clearly show it was in operating in 16bit mode; I think the user was confused that the saturation ADU was staying at 255 and not being automatically set to 65535. They were also switching PHD2 hardware profiles so there is that to consider - fog of confusion is more likely.

So this boils down to 2 things:

  1. A faulty QHY ASCOM driver that always reports a MaxADU of 255 unless manually edited in the driver's ASCOM profile. This is a case of garbage-in, garbage-out from PHD2's perspective.
  2. The QHY native driver doesn't make it clear which bit output the camera is operating in. The user was probably used to seeing this based on his experience with his ZWO camera, where the ZWO native driver implements a setup dialog that permits the user to directly specify 8 or 16bit output.

Digging around in the QHY native driver, I found it would be useful to bring the driver code up to date with camera features, give the user some configuration options, and certainly directly assert the bit mode that the camera is expected to operate in.

agalasso commented 1 month ago

thanks for that info @daleghent very interesting! I agree it would be great to bring the driver code up to date, and we'd be happy to take your pr whenever you feel it is ready

daleghent commented 1 month ago

@agalasso thanks. I think this means we should really warn people away from using the QHY ASCOM driver. As for the commits, I'm going to let this soak a little as I have some other time-critical stuff to finish over the next few days. When I post up the PR, do you want all the commits squashed?

agalasso commented 1 month ago

When I post up the PR, do you want all the commits squashed?

Either way is fine. When we merge we will squash automatically, and when reviewing I don't look at the individual commits anyway. For big stuff that touches multiple areas that should be delivered incrementally, separate stacked prs are ideal (and we would request that). But I don't think that's the case here.

daleghent commented 1 month ago

@agalasso or @bwdev01 , Is there a handy way to get the index value of the selected camera before it's connected to? I want to refine the camera's setup options and enable/disable them in the UI in accordance with the selected camera's features, but it's not clear how I can access this information from with the driver's setup dialog in absence of a connection being active..

agalasso commented 1 month ago

Unfortunately the index value is not a stable identifier: it can change if the camera is plugged in to a different USB port, and it I'm not even sure it is guaranteed to stay the same across machine reboots or plugging in other USB devices. When the user connects to a camera we use the driver-provided camera id (typically a camera serial number -- see EnumCameras) and associate that with the Equipment Profile so that the same camera is connected to every time even if the index changes. The upshot is that you should be able to just save whatever settings you need in the profile (pConfig->Profle). For an example see CameraToupTek::ShowPropertyDialog(). For enabling/disabling features in the ui before the camera is connected one idea that comes to mind would be to connect (OpenQHYCCD -- not Camera::Connect) right when the settings dialog is opened and get access to whatever camera properties you need there. The other idea would be to change the camera's property dialog type to "when connected" (PropertyDialogType = PROPDLG_WHEN_CONNECTED) -- that will cause the ui to only enable the settings dialog after the camera is connected.

daleghent commented 4 weeks ago

Ah ok. QHY's SDK offers the ability to see if a camera of a given index number (as returned by its scan function) has a particular option available to it without having to open & initialize the camera first - GetQHYCCDBeforeOpenParam(). The tooltips for the options are worded to explain that a given option may not be available on the camera in question. Perhaps this is something to revisit at a later point.

daleghent commented 4 days ago

All work completed