HolyLab / Imagine

A graphical interface for recording with OCPI microscopes.
1 stars 1 forks source link

Rename digital input header entries in .imagine file? #39

Closed Cody-G closed 7 years ago

Cody-G commented 7 years ago

I'm testing out reading of header, .di, and .ai files created with the latest waveform branch. We have a problem because the header fields related to .di files have the same names as those that existed for .ai files. Actually I think it was reasonable for @kdw503 to give the fields the same names, but the problem is that currently we expect the names to be unique when we read the header in the ImagineFormat Julia package:

https://github.com/timholy/ImagineFormat.jl/blob/master/src/ImagineFormat.jl#L148-L196

So to fix the conflict we have two choices:

  1. Change ImagineFormat so that it distinguishes between the bracketed section titles ([ai] and [di])
  2. Change the names of the header fields. We can just add a di prefix to the new fields, i.e. di channel list:. In that case probably we should leave the [ai] field names the same so that we don't break previous code.

I think option 2 is easier, but if there is preference otherwise I'm open to that.

kdw503 commented 7 years ago

I like option 2 too. Then we just add 'di' in front of every field in [di] block.

timholy commented 7 years ago

Sounds good to me too. And if you need to, I'm fine with putting a version number into the .imagine file and having ImagineFormat support multiple versions.

Cody-G commented 7 years ago

Cool! We do have a "header version" field that is currently at version 7. Unless we keep running into more issues I feel like we can get away without making a "breaking" change to the format.

One more question for @kdw503: The channel list and di channel list fields are 0-based indices. Do these simply map onto the used ai and di ports in increasing order? For example, on OCPI-1 analog inputs would map like this:

And on ocpi-1 there is just one dedicated digital input port, so:

And the ocpi-2 di mappings would be:

If I'm wrong about that and the mappings are more complicated, then I think maybe we should consider just using the actual DAQ channel numbers in the channel list and di channel list fields.

kdw503 commented 7 years ago

Now, in ocpi-1 for example, channel list(ai) 0 -> AI0 2 -> AI2 5 -> AI5 channel list(di) 0 -> P0.7

In ocpi-2 and ocpi-lsk, channel list(ai) 0 -> AI0 2 -> AI2 5 -> AI5 channel list(di) 0 -> P0.24 2 -> P0.26 4 -> P0.28

Considering your suggestion, we only need to change di channel list. And, I am ok with that change.

kdw503 commented 7 years ago

One more, do we need to add 'command file name' field when we use waveform control?

Cody-G commented 7 years ago

Sounds good! And yes, I think a 'command file name' field would be useful.

On Jun 26, 2017 9:51 AM, "Dae Woo Kim" notifications@github.com wrote:

One more, do we need to add 'command file name' field?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HolyLab/Imagine/issues/39#issuecomment-311082625, or mute the thread https://github.com/notifications/unsubscribe-auth/AE78hEzhMxYG3CpciH-_Z-ZtLgyNcvqzks5sH8VjgaJpZM4OEemW .

Cody-G commented 7 years ago

I will close this, as I think we reached an understanding. I will add more tools for working with inputs in the next few days.

kdw503 commented 7 years ago

I think we need to use the original numbering for .di channel list. If we use actual port number, we need to calculate the bit position number. For example, to get 'camera1 frame monitor' signal from byte array in .di file, we need to check rig name and find the begging number of digital input according to the rig and substract the value from actual port number. But if we just specify bit position number in the byte array as a channel list number, we can just use it.

And, is it OK to change 'command file name' field name as just 'command file' to make it consistent with other file name field such as "ai data file" and 'di data file".

Cody-G commented 7 years ago

If we use actual port number, we need to calculate the bit position number.

True, but if we use the bit position number instead then we still need to keep track of the mapping from bit position to channel number. From my perspective they are both the same level of difficulty, and we only chose to use the actual port number to be consistent with the way AI channels are specified in the .imagine header. But if it's easier to do it using bit positions with Imagine then that's okay with me.

And, is it OK to change 'command file name' field name as just 'command file' to make it consistent with other file name field such as "ai data file" and 'di data file"

Sure, I was thinking the same!

I have a branch now that can read .ai and .di files. I will wait to create the PR until we decide on how to list the channels in the .imagine file.

kdw503 commented 7 years ago

You right. It looks better. Then let's do the actual port number. The .imagine file I sent yesterday was not applied these changes yet. I think you can just edit the file to follow these changes.

kdw503 commented 7 years ago

How about add another field in [di] tab like di input port base=24 (ocpi-2) di input port base=7 (ocpi-1)?

Cody-G commented 7 years ago

How about just specify another field in [di] tab like

This would force us to use a contiguous set of channels for di in the future. It's okay for now, but it may become inconvenient at some point if an Imagine developer wants to change which ports are di and do

Cody-G commented 7 years ago

In ImagineInterface I use dictionaries a lot. We could have a dictionary with keys that are DI channel numbers and values that are bit position numbers. Each rig can have its own dictionary in a separate file (maybe in the .js files for Imagine).

Cody-G commented 7 years ago

(It may also be useful to have a reverse dictionary mapping bit position => DI channel. I use both here: https://github.com/HolyLab/ImagineInterface/blob/master/src/hardware_constants.jl#L2-L100 and I was planning to use a similar strategy to track DI channel bit positions)

kdw503 commented 7 years ago

Now, I designed di and do channel as contiguous.(di channels are located end of p0 port). Then do you think we need to change this as we can assign input output channel freely?

Cody-G commented 7 years ago

Yes, I know that you chose the current channel assignments carefully so that they will be contiguous. I was just thinking about the more distant future when some users may request to assign the channels freely. I don't think it's a feature that we need to implement now, but I was thinking we could try to keep the design flexible enough that it is possible. It also may be useful if at some point the DAQ must be changed, or if another lab (at another university) wants to use Imagine with a different DAQ after we publish the code this year.

Cody-G commented 7 years ago

I just pushed a cjg/read_inputs branch to ImagineInterface. In that branch I support reading .ai and .di files as ImagineCommands. For testing I used modified version of the files you sent me that use the real DAQ channel names in the "di channel list" field. I can change it to the other convention pretty easily if you prefer. If you want to see the files I'm currently using for testing you can find them in the examples folder there.

kdw503 commented 7 years ago

I chose that design because of some reasons. If we allow to choose input output arbitrarily then....

Cody-G commented 7 years ago

I agree that it is a safety issue in our current situation, and on balance I don't think we should allow free assignment for OCPI2 users. But I think there is still good reason for making it easy for a developer (not a user) to reassign channels in the case that we change the DAQ eventually or another lab wants to use Imagine with a different DAQ. We could still require that there are only 8 DI channels so that the bit-packing does not become more complicated, as you said.

kdw503 commented 7 years ago

OK I understand. If user is not allowed free assignment and the number of digital input channels is limited, I am OK with your idea.

Cody-G commented 7 years ago

OK great! So it's decided that there will be a maximum of 8 DI channels per rig, and these channels will be specified by their actual digital channel number in the .di file. Developers may alter which digital channels are used by changing a configuration file when building Imagine, but users will not have that privilege.