search

open-ephys / onix-bonsai-onix1

Bonsai library for the Open Ephys Onix Acquisition System
https://open-ephys.github.io/onix1-bonsai-docs
MIT License
4 stars 3 forks source link

ManagedFrame.FrameClock should be ManagedFrame.Clock #140

Closed jonnew closed 1 month ago

jonnew commented 1 month ago

https://github.com/neurogears/onix-refactor/blob/260149bdfb441c8d49a606bbfd303728cbc42bd0/OpenEphys.Onix/OpenEphys.Onix/ManagedFrame.cs#L18C26-L18C27

Can I make this change and push to main?

glopesdev commented 1 month ago

I would actually remove ManagedFrame entirely from the package. I don't think it is being used anywhere anymore and is no longer the recommended way to build a data frame type.

glopesdev commented 1 month ago

Alternatively we could make it the common base type of all data frames, but a slight refactoring would be needed, I can bring up a proposal in a PR for tomorrow.

jonnew commented 1 month ago

Its used by the heartbeat. The reason I noticed is because I was looking for a place to document Clock and HubClock which are repeated everywhere in the library.

jonnew commented 1 month ago

Re: refactor -- I don't think we need to make a common base class if there is not a good reason, its a minor thing I noticed during documentation.

glopesdev commented 1 month ago

@jonnew I think there are good reasons to having a common base class for this, as it makes it easier to consistently change the naming conventions, introduce new common properties we may not be exposing right now (e.g. DeviceAddress), and also facilitates documentation efforts since these properties only have to be documented in a single place.

I have opened a PR (#147) with a proposal for this which also refactors the heartbeat data and gets rid of ManagedFrame.