indilib / indi-3rdparty

INDI 3rd Party drivers repository
https://www.indilib.org/devices.html
GNU Lesser General Public License v2.1
129 stars 212 forks source link

Toupcam EFW: "Slot Number" is not stored in the Config file. #946

Closed jctk closed 3 months ago

jctk commented 3 months ago

UPDATED: Description of Note: has been updated to mention the number of physical slots.

Describe the bug The "Slot Number" set in ToupTek EFW's INDI Control Panes is not saved in the Config file. In ToupTek's SDK, the "Slot Number" of ToupTek AFW-M always returns 7. Therefore, if you rely on the "Slot Number" in the SDK, the "Slot Number" will not be correct, and the INDI driver must set the "Slot Number" with the value saved in the Config file.

Note: AFW-M can change the maximum number of physical slots to 5, 7, or 8 by replacing the wheel. However, SDK always returns 7 regardless of wheel type.

To Reproduce Exact steps to reproduce the behavior.

  1. Add Toupcam EFW to profile and connect profile.
  2. Set "Slot Number" to "5" and Save configuration.
  3. Disconnect and Connect "ToupTek EFW".
  4. "Slot Number" is restored to "7"

Expected behavior "Slot Number" is saved in the config and restored when connected to the device.

Desktop (please complete the following information):

knro commented 3 months ago

Thanks, I'm a bit lost. A PR was opened to address this then closed?

jctk commented 3 months ago

I have not fixed this issue. I am hoping the main contributor will fix this issue.

knro commented 3 months ago

Please run update_indi_drivers and test. I have a Touptek filter wheel here but it's not responding to motion commands so I could not test. Please test and let me know how it goes.

jctk commented 3 months ago

The previous code worked fine, but this one does not work correctly.

Problem-A. It no longer sets the filter at the desired position. Not always, but it seems that it is not correctly determining the target and current filter number, as it does not work when one filter ahead is specified, but works when two ahead are specified.

  1. ToupTek's SDK should have the wheel position as a 0-based number.
  2. FP(get_Option(m_Handle, CP(OPTION_FILTERWHEEL_POSITION), &CurrentFilter)) should be CurrentFilter = -1 while the wheel is rotating.

I assume that these two points are not handled correctly.

Problem-B. The maximum number of filter slots must be able to be manually specified as 5,7 or 8. "ToupTek AFW-M" can physically have 5, 7 or 8 filter slots by changing the wheel. However, whether the physical filter slot is 5,7 or 8, the SDK always returns the number of slots as 7. Therefore, the operator must manually specify the number of slots.

jctk commented 3 months ago

It would be better to revert to the previous revision of source code and allow the maximum number of slots to be specified manually.

knro commented 3 months ago

@touptek Is there any way to know the # of slots available without having to manually specify it? I can add this back but wanted to confirm.

jctk commented 3 months ago

I have investigated the cause of Problem-A.

I tentatively added the following API call to the ToupWheel::Connect() function.

FP(put_Option(m_Handle, CP(OPTION_FILTERWHEEL_SLOT), 5));
FP(put_Option(m_Handle, CP(OPTION_FILTERWHEEL_POSITION), -1));

I am using the 5 slot version of ToupeTek AFW-M. The first line calls the API that sets the correct number of slots '5'. The second line calibrates the device by specifying '-1' for the filter slot.

This should work correctly.

jctk commented 3 months ago

Therefore, a mechanism to manually set the maximum number of filters for a device is required.

knro commented 3 months ago

Ok I'm unclear on this function: HRESULT rc = FP(get_Option(m_Handle, CP(OPTION_FILTERWHEEL_POSITION), &CurrentFilter));

Shouldn't CurrentFilter reflects the current filter? If -1, then we know it's rotating, otherwise is a zero-based filter wheel position, right?

jctk commented 3 months ago

Immediately after this code, CurrentFilter++ is done and converted to the INDI numbering system (based on 1), so there is no problem. As a result, CurrentFilter is set to 0 during rotation, and the TimerHit function can determine that rotation is in progress because TargetFilter and CurrentFilter are different. This is because TargetFilter is 1-based.

I have since confirmed that the 0-based handling seems to be coded correctly, so it seems to be ok.

I have already confirmed that the Jasem code works by adding the API calls to set the number of valid slots and the API call to do the calibration. For the calibration, it would be better to call ToupWheel::SelectFilter(0) instead of calling the API directly. This way the necessary member variables, etc. will be updated properly.

jctk commented 3 months ago

The latest code ef78687 will be tested later. If possible, please change to ToupWheel::SelectFilter(0) instead of the following code.

FP(put_Option(m_Handle, CP(OPTION_FILTERWHEEL_POSITION), -1));
knro commented 3 months ago

You mean SelectFilter(1) ?

jctk commented 3 months ago

0, not 1, must be passed as an argument. If 0 is taken as an argument, it is converted to -1 by the API call and calibration is performed.

knro commented 3 months ago

Any issues if the code is left as-is ?

jctk commented 3 months ago

If left as is, SetTimer will not be executed, resulting in the ToupWheel object not being in the correct state after the wheel stops, which is done by TimerHit.

knro commented 3 months ago

Ok please check latest code

jctk commented 3 months ago

Two points should be fixed.

Timer should be stopped in ToupWheel::Disconnect(). Segmentation fault occurs because the device is already closed at the timing when QueryFilter is called from TimerHit.

The message that recommends reconnect in ToupWheel::ISNewSwitch() should be changed from LOG_INFO to LOG_WARN. If possible, it would be better to display the message box in the GUI.

jctk commented 3 months ago

I confirmed this issue has been fixed. Thanks.