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

Feature: Memory Banks (Windows Only) #162

Closed
wants to merge 12 commits into from

Conversation

KingRial
Copy link
Contributor

@KingRial KingRial commented Feb 8, 2020

When collecting HW informations about memory, it's also very usefull to know the physical memory banks and their specs

Quick example:
image

Copy link
Owner

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this quite a bit. Just a few relatively minor naming suggestions inline, @KingRial. Address those and I'll merge this in!

Thanks as always for the awesome contribution!
-jay

memory.go Outdated
@@ -11,9 +11,19 @@ import (
"math"
)

type MemoryBank struct {
Name string `json:"name"`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go ahead and remove the Name field. I don't think it adds anything over the Label and Location fields.

memory.go Outdated
@@ -11,9 +11,19 @@ import (
"math"
)

type MemoryBank struct {
Copy link
Owner

@jaypipes jaypipes Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this MemoryModule, to align with industry terminology:

https://en.wikipedia.org/wiki/Memory_module

I know CIM/WMI uses the term "bank", but generally these things are referred to as "modules" in my experience :)

@@ -21,12 +21,49 @@ type win32OperatingSystem struct {
TotalVisibleMemorySize uint64
}

const wmqlPhysicalMemor = "SELECT BankLabel, Capacity, DataWidth, Description, DeviceLocator, Manufacturer, Model, Name, PartNumber, PositionInRow, SerialNumber, Speed, Tag, TotalWidth FROM Win32_PhysicalMemory"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing "y" at the end of the above variable name :)

PartNumber string
PositionInRow uint32
SerialNumber string
Speed uint32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a future PR (not this one), I'd be interested in adding a Speed field to the MemoryModule struct. Need to have a conversation about what the appropriate data type is, though, so let's leave that for a future commit.

SizeBytes: int64(description.Capacity),
Vendor: description.Manufacturer,
})
//totalPhysicalBytes += description.Capacity
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove above comment please.

@KingRial
Copy link
Contributor Author

KingRial commented Mar 1, 2020

As soon as I'll get back from my travels, I'll follow the suggestions

thetravischannel and others added 11 commits March 16, 2020 19:18
Collecting the correct product UUID
Using private win32 structs and static vmi queries
Added partial windows support to cpu and memory

Issue jaypipes#8
Added partial windows support to block
When collecting HW informations about memory, it's also very usefull to know the physical memory banks and their specs
@jaypipes jaypipes force-pushed the win_memory_feature_banks branch from 3f5dd4f to f90bcd9 Compare March 16, 2020 23:26
@jaypipes
Copy link
Owner

@KingRial in trying to rebase this, I totally goofed it up and ended up git cherry-picking the original commit from you into a new PR here: #172

Closing this one out! :)

@jaypipes jaypipes closed this Mar 16, 2020
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.

3 participants