areaDetector / ADGenICam

areaDetector base class for GenICam cameras.
https://areadetector.github.io/master/ADGenICam/ADGenICam.html
7 stars 16 forks source link

PVs restored from database if autosave not present #5

Closed LeeHudsonDLS closed 4 years ago

LeeHudsonDLS commented 4 years ago

Hi, I noticed a difference in behaviour between aravisGigE and ADGenICam/ADAravis in terms of autosave that I think is undesirable. I've found that aravisGigE used to pull the ADCore PV values (like image size, gain etc) from the camera if an autosave file isn't found whereas ADGeniCam uses the database as the master in this instance. The values in the database don't normally make sense and result in the setting in the camera being overwritten. Is this intentional behaviour? Is it related to https://github.com/areaDetector/ADGenICam/issues/3

MarkRivers commented 4 years ago

I see what you mean.

I think the fix is easy. However, I don't think the problem can be fixed in ADGenICam because it requires the derived class to call ADGenICam::readStatus() as soon as the camera has successfully connected. ADGenICam has no way of knowing when the camera has connected.

Please try adding that call in ADAravis::connectToCamera() if the connection succeeds and see if that fixes the problem.

LeeHudsonDLS commented 4 years ago

It doesn't appear to have fixed it, I added ADGenICam::readStatus(); in the method ADAravis::connectToCamera() just after this->connectionValid = 1;

MarkRivers commented 4 years ago

Did you delete the auto_settings.sav file first? I’ll work on it today.

LeeHudsonDLS commented 4 years ago

Yes I did and it overwrote the camera with the values in the DB file. I can see that ADGenICam::readStatus() requires mGCFeatureSet to be initialised and populated but it looks like this happens some time later after the camera has been connected (looking with a debugger as I don't understand the sequence yet). Calling ADGenICam::readStatus() even at the end of the ADAravis constructor doesn't do anything as that map still isn't populated.

MarkRivers commented 4 years ago

I understand the problem. The asynPortDriver parameters are not yet created when the camera first connects from the constructor, because they are created during iocInit when device support calls drvUserCreate. So the camera values need to be read and stored in the parameter library after drvUserCreate is called, but before autosave overwrites them.

I'll take a look at this today.

MarkRivers commented 4 years ago

I am looking at the code in ADGenICam::drvUserCreate(), and it already does what I was suggesting. was needed.

It looks like many PVs do initialize to the camera values, but some do not. I think we may just need to treat these special cases.

What specific PVs are you seeing not initialize to the camera values when there is no autosave file?

LeeHudsonDLS commented 4 years ago

Specifically the image size was being overwritten, the database default is 1x1.

MarkRivers commented 4 years ago

I believe I have fixed the issue. The problem was that pFeature->read() was not being called in ADGenICam::addADDriverFeatures() when it should have been. I also had to move some code from ADGenICam::drvUserCreate() to ADGenICam::addADDriverFeatures() to ensure that things were done in the correct order. All PVs now seem to initialize correctly to the camera values if there is no autosave file.

Let me know if you see any more problems.

LeeHudsonDLS commented 4 years ago

Great thanks i'll give that a try this morning.

LeeHudsonDLS commented 4 years ago

It looks like the settings are now taken from the camera but I noticed that when I set the exposure via "AcquireTime" in the usual way it doesn't propagate down to the camera. I get the ASYN_TRACE_WARNING message:

Param[ACQ_TIME] GenICamFeature::write: node ExposureTime is not implemented

LeeHudsonDLS commented 4 years ago

To add to the above, mIsImplemented for ACQ_TIME is false.

MarkRivers commented 4 years ago

What kind of camera is this?

MarkRivers commented 4 years ago

I just tested with a Point GreyBlackFlyS and ACQ_TIME (AcquireTime) is working fine for me.

When I type

asynReport 2 ARV1

I see this

      Node name: ExposureTime
          value: 200001.000000
      asynIndex: 77
       asynName: ACQ_TIME
       asynType: 4
  isImplemented: true
    isAvailable: true
     isReadable: true
     isWritable: true

      Node name: ExposureTime
          value: 200001.000000
      asynIndex: 250
       asynName: GC_D_ExposureTime
       asynType: 4
  isImplemented: true
    isAvailable: true
     isReadable: true
     isWritable: true

So the ExposureTime feature exists and is mapped to 2 detector parameters.

What do you see?

I can change AcquireTime and it has the desired affect.

MarkRivers commented 4 years ago

Does your camera perhaps implement ExposureTimeAbs rather than ExposureTime? That should work but I have not tested it.

MarkRivers commented 4 years ago

If you are still having problems please attach the database that was generated from the camera XML file.

LeeHudsonDLS commented 4 years ago

Sorry for the slow reply. The camera i've been using is an Allied Vision Manta MG 235B. The camera implements ExposureTimeAbs. Changing the AqcuireTime worked fine before the changes to fix this issue so it's something in this change. I've attached the database for the camera AVT_Manta_G235B.txt

LeeHudsonDLS commented 4 years ago

To answer you're previous question about the asyn parameters, here is what I get before the changes :

      Node name: ExposureTimeAbs
          value: 200004.000000
      asynIndex: 124
       asynName: GC_D_ExposureTimeAbs
       asynType: 3
  isImplemented: true
    isAvailable: true
     isReadable: true
     isWritable: true

      Node name: ExposureTimeAbs
          value: 200004.000000
      asynIndex: 74
       asynName: ACQ_TIME
       asynType: 3
  isImplemented: true
    isAvailable: true
     isReadable: true
     isWritable: true

And here is what I get after:

      Node name: ExposureTime
          value: Not available
      asynIndex: 74
       asynName: ACQ_TIME
       asynType: 3
  isImplemented: false
    isAvailable: false
     isReadable: true
     isWritable: true

      Node name: ExposureTimeAbs
          value: 200004.000000
      asynIndex: 124
       asynName: GC_D_ExposureTimeAbs
       asynType: 3
  isImplemented: true
    isAvailable: true
     isReadable: true
     isWritable: true

I'm quite new to asyn so i'm not sure what to make of this, is there some link between ACQ_TIME and GC_D_ExposureTimeAbs missing? It appears that when it was in a working state ExposureTime didn't exist at all.

MarkRivers commented 4 years ago

I see the problem. I was assuming CreateFeature() returned null if the feature did not exist, but that is not correct. It creates the feature object, which then must be tested with isImplemented().

I have fixed the error. Please test again.

MarkRivers commented 4 years ago

I just tested ADAravis with an AVT Manta G-507C.

It shows the following:

      Node name: ExposureTimeAbs
          value: 49993.000000
      asynIndex: 77
       asynName: ACQ_TIME
       asynType: 4
  isImplemented: true
    isAvailable: true
     isReadable: true
     isWritable: true

      Node name: ExposureTimeAbs
          value: 49993.000000
      asynIndex: 174
       asynName: GC_D_ExposureTimeAbs
       asynType: 4
  isImplemented: true
    isAvailable: true
     isReadable: true
     isWritable: true

So as with your Manta, this camera implements ExposureTimeAbs and not ExposureTime. Now we don't see the ExposureTime feature, only ExposureTimeAbs, and AcquireTime functions as expected. So I think I have fixed the problem,

LeeHudsonDLS commented 4 years ago

Ok thanks, that looks promising. I'll test it first thing tomorrow and confirm.

LeeHudsonDLS commented 4 years ago

Hi Mark, i've tested this today and it appears to have done the trick, thanks for all your help.

LeeHudsonDLS commented 4 years ago

Sorry I know this is closed but this is somewhat related. I noticed that camera specific PVs such as GC_GevSCPSPacketSize don't have PINI set to "Yes" as default but have PINI autosaved instead. I would imagine the intention is to set PINI to specific parameters that you want the IOC to be the master of (without PINI the values don't get written to the camera). I like the idea of that but can see it may be hard to manage as there are allot of PVs this applies to, how do you manage this? Do you just set them on an camera by camera basis?

MarkRivers commented 4 years ago

If your database has autosave info tags for PINI or any other fields then you are using an old version. The Python script was changed to remove those in R1-1. All of the databases were recreated to remove those info tags as well.

If you want to autosave a GC_ PV from autosave you need to do these things.

LeeHudsonDLS commented 4 years ago

Thanks for the clarification, my understanding was of the version before you did the fixes in this ticket. After doing a merge into our branch I see those changes. Thanks

MarkRivers commented 4 years ago

I just tested that autosave works for GC_* features with the following changes in iocAravis. It adds the GC_Gamma feature to auto_settings.req and does a dbpf on the .PROC field of the GC_Gamma record in the startup script. It works fine, restarting the IOC replaces the current GC_Gamma value with the autosaved value.

corvette:aravisIOC/iocBoot/iocAravis>git diff .
diff --git a/iocs/aravisIOC/iocBoot/iocAravis/auto_settings.req b/iocs/aravisIOC/iocBoot/iocAravis/auto_settings.req
index 37de412..a6323d9 100755
--- a/iocs/aravisIOC/iocBoot/iocAravis/auto_settings.req
+++ b/iocs/aravisIOC/iocBoot/iocAravis/auto_settings.req
@@ -1,3 +1,5 @@
 file "aravisCamera_settings.req", P=$(P),  R=cam1:
 file "NDStdArrays_settings.req",  P=$(P),  R=image1:
 file "commonPlugin_settings.req", P=$(P)
+
+$(P)cam1:GC_Gamma
diff --git a/iocs/aravisIOC/iocBoot/iocAravis/st.cmd.BlackflyS_13Y3M b/iocs/aravisIOC/iocBoot/iocAravis/st.cmd.BlackflyS_13Y3M
index 69df90c..35c7118 100755
--- a/iocs/aravisIOC/iocBoot/iocAravis/st.cmd.BlackflyS_13Y3M
+++ b/iocs/aravisIOC/iocBoot/iocAravis/st.cmd.BlackflyS_13Y3M
@@ -25,3 +25,4 @@ epicsEnvSet("GENICAM_DB_FILE", "$(ADGENICAM)/db/PGR_BlackflyS_13Y3M.template")

 < st.cmd.base

+dbpf $(PREFIX)cam1:GC_Gamma.PROC 1
MarkRivers commented 4 years ago

It would be great if you can test the latest release of ADGenICam and ADAravis. What version of base are you using? If it is prior to 3.16.1 you will need to use the master branch of asyn.

LeeHudsonDLS commented 4 years ago

We're on 3.14.12.7 so I would need to use the master branch of asyn. We're currently using ADGenICam R1-1 which is the latest release isn't it? We're using R1-0 of ADAravis and I see there is now an R1-1 so I could test that but I was planning on waiting for aravis0.8. If you would like me to test it I can do that first thing next year as i'm going to be off for 2 weeks as of today.

MarkRivers commented 4 years ago

I was hoping you could test the master branch of asyn, ADGenICam, and ADAravis. That would require aravis 0.7.2 or later.

LeeHudsonDLS commented 4 years ago

I've now got aravis 0.7.2 installed and the master branch of asyn working here. Is there anything in specific you would like me to test or just the basics?

MarkRivers commented 4 years ago

Nothing specific. Test a variety of cameras and see if you have problems.

LeeHudsonDLS commented 4 years ago

Just an update on the testing, i've got a couple of IOCs that have been running for a week or so with no issues using the master branch of asyn, ADGenICam and ADAravis.