Feat: extended rate parsing - #142
Conversation
add basic MCS Decoding test: add additional station info test case with MCS attributes feat: add support for VHT MCS indexes in StationInfo and related parsing logic feat: enhance StationInfo with detailed rate information and VHT support
expose Interface create subtypes that implement the interface
d82f780 to
c608a43
Compare
|
I have added several testcases in |
|
Thanks, I'll try and get to this soon. Maybe @nickgarlis would also like to take a look. |
| func modulationAttributes(rateInfo RateModulationInfo) (attr []netlink.Attribute) { | ||
| // attr = append(attr, netlink.Attribute{Type: unix.NL80211_RATE_INFO_BITRATE, Data: nlenc.Uint16Bytes(uint16(bitrateAttr(s.ReceiveBitrate)))}) | ||
| // attr = append(attr, netlink.Attribute{Type: unix.NL80211_RATE_INFO_BITRATE32, Data: nlenc.Uint32Bytes(bitrateAttr(s.ReceiveBitrate))}) | ||
| switch ri := rateInfo.(type) { |
There was a problem hiding this comment.
Could the RateModulationInfo interface also implement a marshal and unmarshal function so that this becomes a bit easier to read ?
There was a problem hiding this comment.
done, Is this as expected?
| type BaseModulationInfo struct { | ||
| MCS int | ||
| NSS int | ||
| IwDescription string |
There was a problem hiding this comment.
Can IwDescription be built by the Description function instead ?
This would mean that it'd have to be implemented on each type that implements BaseModulationInfo. If you also turned those attributes into their own types, you could use their String methods.
Personally, I prefer to keep the deserialization/serialization logic focused on fields returned/sent from/to the kernel.
There was a problem hiding this comment.
Sure, that would be an option,
I tried to follow closely the c implementation in iw since there the sting is build on the fly as the netlink atributes come in. Creating my own Description Function could not retain that infformation and only make some "nice to read and close to iw format"-String but no iw compatible string. On the other hand, iw warns you every time to not parse it's output and not rely on output format.
But no strong opinion on my side, Cleaning up the parsing is also a nice benefit, therefore I tend to follow your suggestion if no-one intervenes.
There was a problem hiding this comment.
Is there any information in the raw netlink message that iw would include in the output string but that doesn't have a corresponding field in the struct? If not, the Stringer should be able to replicate iw's output. Right ?
| iwDescription += fmt.Sprintf(" VHT-MCS %d", vhtModulationInfo.MCS) | ||
| case unix.NL80211_RATE_INFO_40_MHZ_WIDTH: | ||
| channelWidth = ChannelWidth40 | ||
| iwDescription += " 40MHz" |
There was a problem hiding this comment.
ChannelWidth implements a String function which seems to be returning the same values. Can it be reused ?
There was a problem hiding this comment.
same discussion as above. Following the ìw` implementation gives slightly different String (no explicit 20MHz Channels in the description)
Good Place to clean up, if we drop the "lets-keep-the-descrition-cloase-to-iw"-premise
|
after merging in changes from release 0.8.0 I will work on the suggestions from the reviewers in the next few days |
|
@nickgarlis or @SuperQ Otherwise I will just do that and we can finalize this PullRequest |
|
last 2 commits implement the String Method on the objects itself. |
|
|
||
| func channelWithAttributes(cw ChannelWidth) (attr []netlink.Attribute) { | ||
| switch cw { | ||
| // case ChannelWidth20NoHT: |
There was a problem hiding this comment.
intentionally left in, since we don't do anything for those two choices from the set of all Possible Channel-Widths. Could probably be more explicit like
case ChannelWidth20NoHT:
return attr
case ChannelWidth20:
return attr| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := modulationAttributes(tt.in) | ||
| if diff := cmp.Diff(tt.want, got); diff != "" { |
There was a problem hiding this comment.
I am not sure I understand the purpose of this test. It seems like you're testing modulationAttributes which is part of the test suite. Did you mean to do something like this instead ?
attrs := modulationAttributes(tc.in)
got, err := parseRateInfo(attrs)
if diff := cmp.Diff(tt.want, got); diff != "" {
t.Errorf("mismatch (-want +got):\n%s", diff)
}Also, I appreciate the dynamic test fixtures since that means the tests can be endian-agnostic but I wonder whether that's relevant here. If not, should hard-coded byte fixtures be considered instead ? No strong opinions here.
Sorry for not asking this earlier. I must have thought marshaling was part of client_linux.go
There was a problem hiding this comment.
Hey, thanks for finding this one.
I've updated the test to do the round-trip:
- build attributes via modulationAttributes(tc.in)
- marshal them
- feed them into parseRateInfo
- and assert the resulting ModulationType/Modulation match expectations.
I kept the original raw-attribute assertion too (renamed wantAttrs) since it's still useful for testing the test-file scope function modulationAttributes .
| var info rateInfo | ||
| var rateinfo RateInfo | ||
| // initialize with unknown values | ||
| htModulationInfo := HTModulationInfo{BaseModulationInfo: BaseModulationInfo{MCS: -1, NSS: -1}} |
There was a problem hiding this comment.
nit: instead of initializing with -1 for MCS and NSS could this be changed to something like
var ht *HTModulationInfo
var vht *VHTModulationInfo
var he *HEModulationInfo
var eht *EHTModulationInfoThen assign lazily like
if vht == nil {
vht = &VHTModulationInfo{}
}
vht.MCS = int(attr.Uint8())And finally do
switch {
case eht != nil:
rateinfo.ModulationInfo = eht
case he != nil:
rateinfo.ModulationInfo = he
case vht != nil:
rateinfo.ModulationInfo = vht
case ht != nil:
rateinfo.ModulationInfo = ht
}There was a problem hiding this comment.
I think this pattern breaks on the following Attribute:
case unix.NL80211_RATE_INFO_SHORT_GI:
htModulationInfo.ShortGI = true
vhtModulationInfo.ShortGI = trueBoth Modulation Types use the same Netlink Attribute and we cannot rely on the order of the attributes in the Netlink message. Therefore it is possible that we get this attribute before we get an attribute that defines the modulation.
Initializing both breaks the selection in the switch type != nil
But extracting shortGI in a temp-variable and assigning it in the switch-block would be a possible solution for that.
Nevertheless since we cannot predict the order of the attributes, the EHT case get's quite ugly and bad to read:
case unix.NL80211_RATE_INFO_EHT_MCS:
if eht == nil {
eht = &EHTModulationInfo{}
}
eht.MCS = int(a.Data[0])
case unix.NL80211_RATE_INFO_EHT_NSS:
if eht == nil {
eht = &EHTModulationInfo{}
}
eht.NSS = int(a.Data[0])
case unix.NL80211_RATE_INFO_EHT_GI:
if eht == nil {
eht = &EHTModulationInfo{}
}
eht.GI = int(a.Data[0])
case unix.NL80211_RATE_INFO_EHT_RU_ALLOC:
if eht == nil {
eht = &EHTModulationInfo{}
}
eht.RUAlloc = int(a.Data[0])Let's keep the original implementaion, ok?
| type BaseModulationInfo struct { | ||
| MCS int | ||
| NSS int | ||
| IwDescription string |
There was a problem hiding this comment.
Is there any information in the raw netlink message that iw would include in the output string but that doesn't have a corresponding field in the struct? If not, the Stringer should be able to replicate iw's output. Right ?
|
Hey @nickgarlis Thanks for all the feedback. For me the main Functions I will use are: |
|
Hi @lukas-mbag Sorry for the late response. I think it's all looking good now. It's just this comment that I think is pending. Thanks for addressing the stringer implementation.
Hope you've been enjoying the weather 😎 🌞 |
|
Hi @nickgarlis Last comment has been committed and pushed. Thanks for your time for the review! @SuperQ Any comment from your side or are we ready for the merge? |
This Pull Request makes the RateInfo available for the user.
I am looking for feedback (@SuperQ ?)on the API-Design for the User of this Library. If nobody objects, I will add in testcases and cleanup the documentation before un-drafting this Pull Request.
My Usecase
I want to access the MCS-Index and the number of Spacial Stream (NSS) for RX and TX of the current Connection together with a string description similar to the one provided by the
iw wlan0 linkcommand (i.e.rx bitrate: 173.3 MBit/s VHT-MCS 8 short GI VHT-NSS 2).Changes
RateModulationInfowith the following Methodes:GetMCS() intGetNSS() intDescription() stringWifiGeneration() stringHTModulationInfo,VHTModulationInfo,HEModulationInfoandEHTModulationInforateInfofromclient_linux.gotowifi.goand make it publicRateModulationInfoInterface to theRateInfostruct. Also addChannelWidthandModulationTypeparseRateInfo(b []byte) (*RateInfo, error)function to populate the new fieldsRateInfo(for Rx and Tx) to theStationInfoStructCaveats
RateInfoStruct. If user wants to use parts of the types that are not in the Interface, they have type-cast the concrete struct to the desired type. MCS. NSS, Description and WiFiGeneration are available for all TypesDescription()can differ from theiwoutput (i.e. in my real-world testcase the "shortGI" string is placed at the End of the string not in-between MCS and NSS as in theiwoutput)Station Infothere is now duplicate Information (the bitrate itself) is available directly viaStationInfo.ReceiveBitrateand is also Part of the RateInfo TypeStationInfo.ReceiveRateInfo.Bitrate. This keeps the API of Station Info backwards-compatible but also groups it with the rest of the rate and modulation info.The program was tested solely for our own use cases, which might differ from yours.
Lukas Raffelt < lukas.raffelt@mercedes-benz.com > on behalf of Mercedes-Benz Tech Innovation GmbH, Provider Information
Licensed under MIT