[bsteinb]

See #53

[brayniac]

Should the bound here be Borrow instead? An AI review found that Deref<Target = FieldValueRequest> rejects owned requests, since FieldValueRequest doesn't implement Deref:

let v = vec![FieldValueRequest::id(FieldId(1))];
d.field_values_for(&v);       // compiles
d.field_values_for(v.iter()); // compiles
d.field_values_for(v);        // E0277
d.field_values_for(ids.iter().map(|i| FieldValueRequest::id(i.clone()))); // E0277

The last one is how I'd expect callers to migrate from &[FieldId]. With

I::Item: std::borrow::Borrow<FieldValueRequest>,

and let req = req.borrow(); at the top of the map closure, all four compile (checked against this head; the ops::Deref import becomes unused).

[bsteinb]

Sounds good to me. I have changed the bound.

[brayniac]

I would add a caution here that None doesn't mean "no scope": it sends 0, which for NVLink fields is link 0 and for power fields is GPU-only scope. I do think the Option type here could be a little deceptive/surprising.

[bsteinb]

I think what I was trying to express here is the fact that queries for certain fields only make sense in combination with a scope while other fields are not scoped at all. A more rigid way to express this would require encoding which field falls in which category and then either make constructors of FieldValueRequest fallible or probably even better turn FieldValueRequest into a proper enum with the field ID as discriminant and scope as mandatory field on those variants that need it. Looking at https://docs.nvidia.com/deploy/nvml-api/latest/api/group__nvmlFieldValueEnums.html, oof, not sure it is worth the trouble.

If we want to stay closer to this design, would it make more sense to you to have the scope_id field be non-optional and instead have ScopeId implement Default (which would be 0)?

[brayniac]

@bsteinb yeah, I think my preference is that ScopeId is non-optional here and derives both Default and Copy. I think once that's done, this will be ready for merge. Thanks!

[bsteinb]

Done, I have made the scope ID field mandatory and touched up the documentation. FieldId and ScopeId now both implement Cope (though not FieldValueRequest as other structs in the file also do not implement it) and ScopeId also has a default value now.

[brayniac]

The CHANGELOG entry doesn't match the format of the recent releases (sentences, **Breaking**: on API breaks, PR number). Something like:

## [Unreleased]

### Changed

* **Breaking**: `Device::field_values_for()` takes an iterator of `FieldValueRequest` (a field ID plus an optional scope ID) instead of `&[FieldId]`; wrap existing IDs with `FieldValueRequest::id()` (#55)
* **Breaking**: `FieldValueSample` has a new public field, `scope: ScopeId` (#55)

### Added

* `ScopeId` and `FieldValueRequest` in `structs::device`, for querying scoped fields such as per-link NVLink counters and module-scope power (#55)

[bsteinb]

Ah, I should have had a closer look at the diff context here while rebasing. The difference is quite obvious.