indigo-astronomy / indigo

INDIGO is a system of standards and frameworks for multiplatform and distributed astronomy software development designed to scale with your needs.
http://www.indigo-astronomy.org
Other
139 stars 64 forks source link

Incorrect JSON response for ASCOM Alpaca request for configured devices #504

Closed rwg0 closed 7 months ago

rwg0 commented 9 months ago

Hi,

I've been testing out the 'Alpaca Agent' functionality to try to get Indigo devices working in an ASCOM application (on the ASCOM side I am using the ASCOM Library - https://github.com/ASCOMInitiative/ASCOMLibrary). Unfortunately it fails at the device discovery stage because the ASCOM side code is expecting a number for the 'DeviceNumber' field, but instead the Alpaca Agent returns a string with a number in it - ie

Desired result

"DeviceNumber": 3

Actual output

"DeviceNumber": "3"

This looks pretty simple to fix fortunately - just removing the quotes around the %d for DeviceNumber in this line

https://github.com/indigo-astronomy/indigo/blob/58abd8e8a39f248f25d8a7c0faf8cd0dd4c97dbb/indigo_drivers/agent_alpaca/indigo_agent_alpaca.c#L252

For reference, the API spec for this is

ConfiguredDevicesResponse{
Value   [

Array of device configuration objects.
{
DeviceName  string

A short name for this device that a user would expect to see in a list of available devices.
DeviceType  string

One of the supported ASCOM Devices types such as Telescope, Camera, Focuser etc.
DeviceNumber    integer($uint32)
minimum: 0
maximum: 4294967295

The device number that must be used to access this device through the Alpaca Device API.
UniqueID    string

A string representation of a random value that uniquely identifies this ASCOM device; the random value should have a minimum of 48bits of randomness. Where possible a UUID / GUID should be used, but this is not mandatory.

(you can see the swagger for this here : https://ascom-standards.org/api/?urls.primaryName=ASCOM%20Alpaca%20Management%20API)

I will have a go at building a tweaked version just to make sure there aren't any other 'gotchas' past this one.

cheers,

Robin

rwg0 commented 9 months ago

Ah, the client side code also doesn't like a trailing comma in the JSON response after a list of objects. Although the code (https://github.com/indigo-astronomy/indigo/blob/58abd8e8a39f248f25d8a7c0faf8cd0dd4c97dbb/indigo_drivers/agent_alpaca/indigo_agent_alpaca.c#L253-L257) looks like it will only add a comma if there are more devices to come, I suspect that there are some device entries with 'device_type' of NULL, meaning that a comma gets added but no more data is then appended.

The code below works correctly as it only appends a comma before the second and subsequent items, but of course there are lots of ways to solve this problem

        bool comma=false;
        while (alpaca_device) {
                if (alpaca_device->device_type) {
                        if (comma)
                        {
                                buffer[index++]=',';
                                buffer[index++]=' ';
                        }
                        comma = true;
                        index += snprintf(buffer + index, INDIGO_BUFFER_SIZE - >
                        alpaca_device = alpaca_device->next;
                } else {
                        alpaca_device = alpaca_device->next;
                }
        }
rumengb commented 9 months ago

Hi, This used to work, and as far as I remember it had to be done this way for the same reason. Maybe the ALPACA team fixed it on their end rendering third-party clients broken. By the way, there are several things in the ALPACA agent that are not implemented as described in the ALPACA documentation simply because they did not work as described. Hence, we adjusted our code to make it work. However, I think we used Ascom Platform not ASCOM Library. I am updating and I will test it.

rwg0 commented 9 months ago

Hmm, well in that case I can suggest to the ASCOM team that they use more forgiving settings on the JSON parsing, which might do the trick. I will investigate that possibility to see if it works from a code point of view, since I have the code for both the ASCOM Library and Indigo building now.

rumengb commented 9 months ago

Just checked AScom platform against alpaca agent exporting several devices, I checked the CCD camera:

ConformanceCheck ASCOM Device Conformance Checker Version 6.6.8048.17861, Build time: 1/13/2022 8:55:24 AM ConformanceCheck Running on: ASCOM Platform 6.6 SP2 6.6.2.4195

ConformanceCheck Driver ProgID: ASCOM.AlpacaDynamic3.Camera

Error handling Error number for "Not Implemented" is: 80040400 Error number for "Invalid Value 1" is: 80040405 Error number for "Value Not Set 1" is: 80040402 Error number for "Value Not Set 2" is: 80040403 Error messages will not be interpreted to infer state.

14:42:22.445 Driver Access Checks OK
14:42:24.847 AccessChecks OK Successfully created driver using late binding 14:42:25.685 AccessChecks OK Successfully connected using late binding 14:42:25.701 AccessChecks INFO The driver is a COM object 14:42:26.783 AccessChecks INFO Driver does not expose interface ICamera 14:42:26.798 AccessChecks INFO Driver does not expose interface ICameraV2 14:42:26.836 AccessChecks OK Successfully created driver using driver access toolkit 14:42:27.668 AccessChecks OK Successfully connected using driver access toolkit

Conform is using ASCOM.DriverAccess.Camera to get a Camera object 14:42:28.126 ConformanceCheck OK Driver instance created successfully 14:42:29.016 ConformanceCheck OK Connected OK

Common Driver Methods 14:42:29.070 InterfaceVersion OK 3 14:42:29.117 Connected OK True 14:42:29.155 Description OK CCD Imager Simulator 14:42:29.201 DriverInfo OK ASCOM Dynamic Driver v6.6.2.4195 - REMOTE DEVICE: indigo_ccd_simulator 14:42:29.240 DriverVersion OK 2.0.0.19 14:42:29.287 Name OK CCD Imager Simulator 14:42:29.340 CommandString INFO Conform cannot test the CommandString method 14:42:29.340 CommandBlind INFO Conform cannot test the CommandBlind method 14:42:29.340 CommandBool INFO Conform cannot test the CommandBool method 14:42:29.355 Action INFO Conform cannot test the Action method 14:42:29.370 SupportedActions OK Driver returned an empty action list

Can Properties 14:42:29.488 CanAbortExposure OK True 14:42:29.501 CanAsymmetricBin OK False 14:42:29.502 CanGetCoolerPower OK True 14:42:29.518 CanPulseGuide OK False 14:42:29.524 CanSetCCDTemperature OK True 14:42:29.524 CanStopExposure OK False 14:42:29.543 CanFastReadout OK False

Pre-run Checks

Last Tests 14:42:29.757 LastExposureDuration OK LastExposureDuration correctly generated a COM exception before an exposure was made 14:42:29.773 LastExposureStartTime OK LastExposureStartTime correctly generated a COM exception before an exposure was made

Properties 14:42:29.874 MaxBinX OK 1 14:42:29.922 MaxBinY OK 1 14:42:29.960 BinX Read OK 1 14:42:30.010 BinY Read OK 1 14:42:30.126 BinX Write OK Exception correctly generated on setting BinX to 0 14:42:30.173 BinX Write OK Exception correctly generated on setting BinX to 2 14:42:30.226 BinY Write OK Exception correctly generated on setting BinY to 0 14:42:30.274 BinY Write OK Exception correctly generated on setting BinY to 2 14:42:30.376 BinXY Write OK Successfully set symmetric xy binning: 1 x 1 14:42:30.492 CameraState OK cameraIdle 14:42:30.562 CameraXSize OK 1600 14:42:30.610 CameraYSize OK 1200 14:42:30.680 CCDTemperature OK 25 14:42:30.727 CoolerOn Read OK False 14:42:30.827 CoolerOn Write OK Successfully changed CoolerOn state 14:42:30.880 CoolerPower OK 0 14:42:30.928 ElectronsPerADU OK 1 14:42:30.995 FullWellCapacity OK 65536 14:42:31.052 HasShutter OK False 14:42:31.099 HeatSinkTemperature OK Optional member threw a PropertyNotImplementedException exception. 14:42:31.167 ImageReady OK False 14:42:31.234 ImageArray OK Exception correctly generated when ImageReady is false 14:42:31.266 ImageArrayVariant OK Exception correctly generated when ImageReady is false 14:42:31.283 IsPulseGuiding OK False 14:42:31.330 MaxADU OK 65536 14:42:31.401 NumX Read OK 1600 14:42:31.499 NumX write OK Successfully wrote 800 14:42:31.515 NumY Read OK 1200 14:42:31.603 NumY write OK Successfully wrote 600 14:42:31.619 PixelSizeX OK 5.2 14:42:31.675 PixelSizeY OK 5.2 14:42:31.722 SetCCDTemperature Read OK 0 14:42:31.839 SetCCDTemperature Write OK Successfully wrote 0.0 14:42:34.331 SetCCDTemperature Write INFO Setpoint lower limit found in the range -270.000 to -274.999 degrees 14:42:34.835 SetCCDTemperature Write INFO Setpoint upper limit found in the range 50.000 to 54.999 degrees 14:42:34.904 StartX Read OK 0 14:42:34.991 StartX write OK Successfully wrote 800 14:42:35.005 StartY Read OK 0 14:42:35.092 StartY write OK Successfully wrote 600 14:42:35.121 SensorType Read OK Monochrome 14:42:35.138 BayerOffsetX Read OK Sensor type is Monochrome and a PropertyNotImplementedException exception was generated as expected 14:42:35.159 BayerOffsetY Read OK Sensor type is Monochrome and a PropertyNotImplementedException exception was generated as expected 14:42:35.159 ExposureMax Read OK 10000 14:42:35.175 ExposureMin Read OK 0 14:42:35.191 ExposureMin OK ExposureMin is less than or equal to ExposureMax 14:42:35.207 ExposureResolution Read OK 0 14:42:35.207 ExposureResolution OK ExposureResolution is less than or equal to ExposureMax 14:42:35.222 FastReadout Read OK Optional member threw a PropertyNotImplementedException exception. 14:42:35.278 FastReadout Write OK Optional member threw a PropertyNotImplementedException exception. 14:42:35.292 GainMin Read OK 0 14:42:35.307 GainMax Read OK 500 14:42:35.322 Gains Read OK Optional member threw a PropertyNotImplementedException exception. 14:42:35.339 Gain Read OK 100 14:42:35.339 Gain Read OK Gain, GainMin and GainMax can be read OK while Gains throws an exception - the driver is in "Gain Value" mode. 14:42:35.393 Gain Write OK Successfully set gain minimum value 0. 14:42:35.462 Gain Write OK Successfully set gain maximum value 500. 14:42:35.511 Gain Write OK InvalidValueException correctly generated for gain -1, which is lower than the minimum value. 14:42:35.563 Gain Write OK InvalidValueException correctly generated for gain 501 which is higher than the maximum value. 14:42:35.581 PercentCompleted Read OK Optional member threw a PropertyNotImplementedException exception. 14:42:35.596 ReadoutModes Read OK RAW 1600x1200 14:42:35.611 ReadoutModes Read OK RAW 800x600 14:42:35.611 ReadoutModes Read OK RAW 400x300 14:42:35.627 ReadoutMode Read OK 0 14:42:35.642 ReadoutMode Index OK ReadReadoutMode is within the bounds of the ReadoutModes ArrayList 14:42:35.642 ReadoutMode Index INFO Current value: RAW 1600x1200 14:42:35.664 SensorName Read OK The driver returned an empty string 14:42:35.680 OffsetMin Read OK 0 14:42:35.680 OffsetMax Read OK 10000 14:42:35.696 Offsets Read OK Optional member threw a PropertyNotImplementedException exception. 14:42:35.711 Offset Read OK 0 14:42:35.727 Offset Read OK Offset, OffsetMin and OffsetMax can be read OK while Offsets throws an exception - the driver is in "Offset Value" mode. 14:42:35.783 Offset Write OK Successfully set offset minimum value 0. 14:42:35.829 Offset Write OK Successfully set offset maximum value 10000. 14:42:35.884 Offset Write OK InvalidValueException correctly generated for offset -1, which is lower than the minimum value. 14:42:35.946 Offset Write OK InvalidValueException correctly generated for offset 10001 which is higher than the maximum value. 14:42:35.946 SubExposureDuration OK Optional member threw a PropertyNotImplementedException exception. 14:42:36.099 SubExposureDuration write OK Optional member threw a PropertyNotImplementedException exception.

Methods 14:42:36.232 AbortExposure OK No exception generated when camera is already idle 14:42:36.286 PulseGuide OK CanPulseGuide is false and PulseGuide is not implemented in this driver 14:42:36.333 StopExposure OK CanStopExposure is false and .NET exception correctly generated

Take image full frame 1 x 1 bin 14:42:38.943 StartExposure OK Asynchronous exposure found OK: 2 seconds 14:42:38.957 LastExposureDuration OK LastExposureDuration is: 2 seconds 14:42:38.974 LastExposureStartTime OK LastExposureStartTime is correct to within 2 seconds: 2023-10-02T21:42:36 UTC 14:42:39.191 ImageArray OK Successfully read 32 bit integer array (1 plane) 1600 x 1200 pixels in 0.206s 14:42:41.216 ImageArrayVariant OK Successfully read variant array (1 plane) with System.Int32 elements 1600 x 1200 pixels in 2.011s

StartExposure error cases 14:42:41.620 StartExposure OK Exception correctly generated for negative duration 14:42:41.977 StartExposure OK Exception correctly generated for X size larger than binned chip size, Bin 1x1 14:42:42.329 StartExposure OK Exception correctly generated for Y size larger than binned chip size, Bin 1x1 14:42:42.704 StartExposure OK Exception correctly generated for X start outside binned chip size, Bin 1x1 14:42:43.051 StartExposure OK Exception correctly generated for Y start outside binned chip size, Bin 1x1

Post-run Checks 14:42:43.324 PostRunCheck OK Camera returned to initial cooler temperature

Conformance test complete

No errors, warnings or issues found: your driver passes ASCOM validation!!

rumengb commented 9 months ago

And the discovery works just fine: image

rwg0 commented 9 months ago

Ok, apologies for this one then - it looks like it is inconsistency at the ASCOM end - their new ASCOM library is obviously less forgiving than the older code in the ASCOM platform :(

rumengb commented 9 months ago

Actually I am pretty sure we did it this way because ASCOM Platform did not work without double quotes. However I have a strong suspicion that ALPACA is not ready for production and is rarely used. There is only one native Alpaca driver for Optec focuser all other devices shold be exported via Ascom remote or indigo....

DanielVanNoord commented 9 months ago

Hello @rumengb. If you are testing if your Alpaca API is correct I highly recommend that you use ConfromU instead of Conform. You can find the latest releases here: https://github.com/ASCOMInitiative/ConformU. ConformU is Alpaca aware (Alpaca drivers should be directly tested through ConformU), cross platform, and runs a fair number of extra tests and more specifically enforces the specification then the ASCOM Dynamic Drivers do.

The Dynamic Drivers that are built into the platform were made for compatibility, ConformU was made for testing.

If you pass ConformU in strict mode (available in the settings) your API is likely to be quite correct. We also add explicit tests for new issues as they are discovered.

rumengb commented 9 months ago

@DanielVanNoord - Same result :) camera is docovered and passed with flying colours: image

rwg0 commented 9 months ago

Ah, I see where we are getting confused. The screenshot above shows that the camera is being selected via the ASCOM Platform dynamic setup of Alpaca devices and the older COM based ASCOM Platform, not via direct Alpaca discovery in ConformU.

Go to the 'Alpaca Discovery Map' page.

With the currently released Indigo (2.0-246), you will find no discovered devices

image

With a patch to the JSON creation to take the device numbers out of quotes and suppress the trailing comma

image

This reflects that the ASCOM Platform is more lenient about the JSON reading than the ASCOM Library code used in ConformU.

My take is that the best approach would be to correct the JSON response to match specification and to change the ASCOM Library to be more lenient for compatibility with versions that are already installed, but I'm viewing this with an App developer's view of 'wanting stuff to just work for the end users'.

cheers,

Robin

DanielVanNoord commented 9 months ago

@rumengb It looks like you ran the test against the already created COM Dynamic Driver that was on your system. The Dynamic Drivers are fairly lenient, plus if it was already created it wouldn't have issues with discovery. If you don't mind try discovering them again through ConformU and run your tests directly as an Alpaca device. ConformU supports testing both COM Drivers and Alpaca drivers directly. The huge advantage of testing Alpaca drivers directly is that it tests for a bunch of possible issues that the COM Drivers may be hiding. If you run the Alpaca Tests (and the Alpaca Discovery Map tests) as Robin did it should show the error.

rwg0 commented 9 months ago

I have added a pull request with my changes that put the response to the ConfiguredDevices API into alignment with the API spec. For me that works with both the base ASCOM Platform discovery and the newer ASCOM library code

rumengb commented 9 months ago

It did not work as is, I removed the quites and I got this: image and image

But I still do not understand the coma issue. i can not reproduce it....

rwg0 commented 9 months ago

I didn't debug through the comma issue (a long time since I have debugged any C/C++ on linux). However, my guess is that the linked list of alpaca_devices may sometimes have one or more empty entries at the end that are non-NULL but have a NULL alpaca_device->device_type.

That would cause the comma to be added when processing the last 'live' item in the list since the ->next pointer is not null. However if you go round the loop again and find that ->device_type is null, the comma that has been added remains on the string.

Below is the JSON I am getting from 2.0-246, which has the trailing comma after the last array item

{ "Value": [ { "DeviceName": "Altair ALTAIRGP130M (guider)", "DeviceType": "Telescope", "DeviceNumber": "1", "UniqueID": "775684E2-D406-4B47-8372-3362E338B455" }, { "DeviceName": "Altair ALTAIRGP130M", "DeviceType": "Camera", "DeviceNumber": "0", "UniqueID": "775684E2-0797-4225-8415-9554D475B455" }, { "DeviceName": "Mount Simulator (guider)", "DeviceType": "Telescope", "DeviceNumber": "3", "UniqueID": "47B431C1-E184-493C-8184-C73776B76797" }, { "DeviceName": "Mount Simulator", "DeviceType": "Telescope", "DeviceNumber": "2", "UniqueID": "2567C757-B722-4850-8747-C73776B76797" }, { "DeviceName": "CCD File Simulator", "DeviceType": "Camera", "DeviceNumber": "11", "UniqueID": "A3E29322-A407-437B-8622-850747C73776" }, { "DeviceName": "DSLR Simulator", "DeviceType": "Camera", "DeviceNumber": "10", "UniqueID": "84851575-2285-4074-87C7-3776B7679700" }, { "DeviceName": "CCD Guider Simulator (AO)", "DeviceType": "Telescope", "DeviceNumber": "9", "UniqueID": "23423345-83A5-4547-82C2-1622850747C7" }, { "DeviceName": "CCD Guider Simulator (guider)", "DeviceType": "Telescope", "DeviceNumber": "8", "UniqueID": "23423345-83A5-4543-8011-C18493C184C7" }, { "DeviceName": "CCD Guider Simulator", "DeviceType": "Camera", "DeviceNumber": "7", "UniqueID": "23423345-83C7-407A-86B6-9722850747C7" }, { "DeviceName": "CCD Imager Simulator (focuser)", "DeviceType": "Focuser", "DeviceNumber": "6", "UniqueID": "23423345-E325-4D41-80A0-21A522C01226" }, { "DeviceName": "CCD Imager Simulator (wheel)", "DeviceType": "FilterWheel", "DeviceNumber": "5", "UniqueID": "23423345-E325-4D41-81D0-8194344447C7" }, { "DeviceName": "CCD Imager Simulator", "DeviceType": "Camera", "DeviceNumber": "4", "UniqueID": "23423345-E347-476D-86B6-9722850747C7" }, ], "ClientTransactionID": 0, "ServerTransactionID": 2 }
DanielVanNoord commented 9 months ago

Alpaca specifies using JSON for data exchange and JSON does not allow trailing commas (https://stackoverflow.com/questions/201782/can-you-use-a-trailing-comma-in-a-json-object and MDN "JSON, however, disallows all trailing commas"). Most parsers will ignore them (several of the ones in the existing Alpaca libraries do but other implementations don't) but some will not. For maximum compatibility with JSON parsing libraries we try to follow the JSON specification as carefully as possible.

rumengb commented 9 months ago

Thank you for the PR, but I have implemented a bit more elegant solution I think :) Thank you for pointing out the issues. Now we have native alpaca tests passing too. Great work!

rwg0 commented 9 months ago

I wasn't sure if the approach of

https://github.com/indigo-astronomy/indigo/blob/270684db3025cc8eed2267c38edfa014cb318e87/indigo_drivers/agent_alpaca/indigo_agent_alpaca.c#L254

was safe. It breaks if you can have a situation where the linked list has

device (device_type != NULL) -> device (device_type == NULL) -> device (device_type != NULL)

In that situation, the writing of the first device will not add a comma because the next device has a null type. The second item will write nothing, so the third will get added without a comma separating it from the first.

I couldn't convince myself from the code whether that situation was possible or not in the linked list, which was why I went for the bool comma=false; approach

rumengb commented 9 months ago

Anyway thank you for the pointing the issue. and thank you for proposing the solution. actually I started writing comments on the pr, one was about the coding style and the second was about the solution but I decided that it was easier to just fix it. thank you again for your help.

On Tue, Oct 3, 2023, 9:28 PM Robin @.***> wrote:

I wasn't sure if the approach of

https://github.com/indigo-astronomy/indigo/blob/270684db3025cc8eed2267c38edfa014cb318e87/indigo_drivers/agent_alpaca/indigo_agent_alpaca.c#L254

was safe. It breaks if you can have a situation where the linked list has

device (device_type != NULL) -> device (device_type == NULL) -> device (device_type != NULL)

In that situation, the writing of the first device will not add a comma because the next device has a null type. The second item will write nothing, so the third will get added without a comma separating it from the first.

I couldn't convince myself from the code whether that situation was possible or not in the linked list, which was why I went for the bool comma=false; approach

— Reply to this email directly, view it on GitHub https://github.com/indigo-astronomy/indigo/issues/504#issuecomment-1745505293, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE5EZBNBKLJGRJPDB2BOW6LX5RKLXAVCNFSM6AAAAAA5P7KUYOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBVGUYDKMRZGM . You are receiving this because you were mentioned.Message ID: @.***>

rumengb commented 9 months ago

fixed with commit 270684db3025cc8eed2267c38edfa014cb318e87