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

Add memory usage monitor support #56

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

jonnew
Copy link
Member

@jonnew jonnew commented Feb 26, 2024

  • Configuration
  • High-level data frame and MemoryUsage reporting node

- Configuration
- High-level data frame and MemoryUsage reporting node
@glopesdev
Copy link
Collaborator

@jonnew this looks great, I would like to merge this but just to confirm the naming. In the datasheet this device is called Memory Usage Monitor although the URL is just memory-usage.

In your implementation you used two different variations of the term, i.e. ConfigureMemoryMonitor (the config node) and MemoryUsage (the data node). Do you have any preference on which term would make most sense? For consistency, I believe the choice should be between the following pairs of node names:

MemoryMonitor

  • ConfigureMemoryMonitor
  • MemoryMonitorData

MemoryUsage

  • ConfigureMemoryUsage
  • MemoryUsageData

MemoryUsageMonitor

  • ConfigureMemoryUsageMonitor
  • MemoryUsageMonitorData

@jonnew
Copy link
Member Author

jonnew commented Feb 27, 2024

Hey cool. I think the first two are fine.

ConfigureMemoryMonitor
MemoryMonitorData

The reason I didnt do this is because AnalogInput didnt have the Data suffix. I guess this will change?

@jonnew
Copy link
Member Author

jonnew commented Feb 27, 2024

Also, I just realized:

        public ulong FrameClock { get; private set; }

        public uint DeviceAddress { get; private set; }

Do these need private set since they are assigned in constructor?

@glopesdev
Copy link
Collaborator

I think the first two are fine.

Great, will edit directly in the PR branch if you don't mind.

The reason I didnt do this is because AnalogInput didnt have the Data suffix. I guess this will change?

Yup, let's think about it, it might make sense to change it.

Do these need private set since they are assigned in constructor?

No need, the syntax with a single get means exactly that, you can still assign in the constructor.

@glopesdev
Copy link
Collaborator

Actually, I will merge this as-is for now, and we can discuss the naming conventions as a whole before release.

@glopesdev glopesdev self-requested a review February 27, 2024 22:14
@glopesdev glopesdev merged commit 0985fbb into open-ephys:main Feb 27, 2024
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.

2 participants