Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New time and geometry #19

Merged

Conversation

FrancaCassol
Copy link
Collaborator

This PR change the default geometry to the version 3 (in ctapipe_extra) which is in the Camera frame, version 2 was wrongly rotated
I also set the r0 trigger_time to the dragon time of the central module (to be changed when we get a correct ucts)

@FrancaCassol
Copy link
Collaborator Author

The problem here is that the correct geometry (version 3) is for the moment only in the master of ctapipe_extra, I will ask to get a new release but I don't know how long it will take. Should I add this geometry somewhere else?

@FrancaCassol
Copy link
Collaborator Author

WARNING: it seems that the CDTS data model changed in the last version of the EvB. I am going to add to this PR the necessary change.

@morcuended
Copy link
Member

See #18

@morcuended
Copy link
Member

Hi @FrancaCassol
This is the new structure according to Julien:

typedef struct
{
    tUInt64 uctsTimeStamp;
    tUInt32 uctsAddress;
    tUInt32 eventCounter;
    tUInt32 busyCounter;
    tUInt32 ppsCounter;
    tUInt32 clockCounter;
    tUInt8 triggerType;                 // For TIB, this is the first 7 bits of SPI
    tUInt8 whiteRabbitResetBusyStatus;  // A few status flags here, see below
    tUInt8 stereoPattern;               // For TIB, this is the next 8 bits of the SPI string
    tUInt8 numInBunch;                  // This information only needed for debugging
    tUInt32 cdtsVersion;                // Should be x.y.z where x is 2bytes and y, z are a byte each
} __attribute__((__packed__)) tExtDevicesCDTSMessage;

@FrancaCassol
Copy link
Collaborator Author

Hi @morcuended,
thanks, I had the luck to get it too. I hope in the future this type of changes manage to follow an official communication channel

@morcuended
Copy link
Member

It would be possible to make it backward compatible? For example, check the version of the CDTS first, and then use the corresponding data model

@FrancaCassol
Copy link
Collaborator Author

obviously, I will use the daq version to correctly read the data

@mdpunch
Copy link

mdpunch commented Dec 16, 2019

Hi @morcuended,
thanks, I had the luck to get it too. I hope in the future this type of changes manage to follow an official communication channel

Hello Franca,
What should be the official communication channel for this information? As I'm not in the LST consortium, I don't know. Julien is in LST, though, but doesn't seem to be on github, so won't get this message unless you tell him in the CPPM lab!
Good Luck,
Michael.

@FrancaCassol
Copy link
Collaborator Author

Hi @mdpunch,

What should be the official communication channel for this information? As I'm not in the LST consortium, I don't know. Julien is in LST, though, but doesn't seem to be on github, so won't get this message unless you tell him in the CPPM lab!

I think you got the point. Don't worry, I think the message arrived.

@morcuended
Copy link
Member

Is the test failing because the current file example_LST_R1_10_evts.fits.fz does not contain the new-named LSTCam-003.camgeom table? Should it be updated then or add another LST file for testing?

@maxnoe
Copy link
Member

maxnoe commented Dec 16, 2019

Is the test failing because the current file example_LST_R1_10_evts.fits.fz does not contain the new-named LSTCam-003.camgeom table?

The CameraGeometry is in ctapipe-extra and has nothing to do with the input data file.

I would suggest that you add the lst camera geometry as a resource file in this repository, as it is clearly required for the lst event source.

(pinging @kosack here)

@FrancaCassol
Copy link
Collaborator Author

I would suggest that you add the lst camera geometry as a resource file in this repository, as it is clearly required for the lst event source.

(pinging @kosack here)

Hi @maxnoe,
that is a good idea.
@rlopezcoto what do you think about that?

@maxnoe
Copy link
Member

maxnoe commented Dec 16, 2019

It's only 13 kB, so basically nothing

@rlopezcoto
Copy link
Contributor

it sounds like a good solution.

I would suggest that you add the lst camera geometry as a resource file in this repository, as it is clearly required for the lst event source.
(pinging @kosack here)

Hi @maxnoe,
that is a good idea.
@rlopezcoto what do you think about that?

@maxnoe maxnoe mentioned this pull request Dec 16, 2019
@maxnoe
Copy link
Member

maxnoe commented Dec 16, 2019

See #20

@mdpunch
Copy link

mdpunch commented Dec 16, 2019

Hi @mdpunch,

What should be the official communication channel for this information? As I'm not in the LST consortium, I don't know. Julien is in LST, though, but doesn't seem to be on github, so won't get this message unless you tell him in the CPPM lab!

I think you got the point. Don't worry, I think the message arrived.

Hi Franca,
Okay, good, but I'd like to know also!
I was thinking it should be put in some data model repository like the one that Etienne Lyard started (and read there rather than hardcoded), but can't find that now,
Good Luck,
Michael.

@FrancaCassol
Copy link
Collaborator Author

Hi @mdpunch,

Okay, good, but I'd like to know also!

this is a big problem in my view, I also could not find the official place where this information is supposed to be. I got the data model one day from Julien (that is my only known official channel) and I had to limit my ambition to at least be informed if the output of the EvB is changing (in order to be able to modify the offline reader). I agree that this is not really satisfactory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants