|
View:
New views
20 Messages
—
Rating Filter:
Alert me
|
| < Prev | 1 - 2 | Next > |
|
|
Bug 602 statusThis is a possible patch for bug 602. Please take a look at it. Thank you all, Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
|
|
|
Re: Bug 602 statusMirko Banchi wrote:
> > > This is a possible patch for bug 602. Please take a look at it. > Hi Mirko, Thanks for submitting the patch. The patch looks good for me but I have some remarks: - In mac-low.cc the same piece of code replacing station->ReportDataFailed() is repeated 3 times using copy & paste, can you create a small function instead or encapsulate the behavior in another routine. You can do the same for ReportDataOk, ReportRtsFailed... -In wifi-remote-station-manager.cc: -line 811, please do not use NS_ASSERT (false) and use instead NS_FATAL_ERROR and include a comprehensive message -Also the code if (ac == AC_UNDEF) .... is repeated several times using copy and paste, sometimes with minor changes like *mqosSsrc.find (ac)->second = 0 instead of ++, please encapsulate this piece of code into a function with comprehensive name -Line 927: //How should we use ac parameter here? I have no answer for that, do you have one? some plans?? Other than that, the patch is cool for me, once you finish fixing the above you will get +1 from me. Someone else has comments about the patch? Best regards Faker Moatamri |
|
|
|
|
|
Re: Bug 602 statusIl giorno 23/ott/09, alle ore 11:40, Faker Moatamri ha scritto: > Mirko Banchi wrote: >> >> >> This is a possible patch for bug 602. Please take a look at it. >> > Hi Mirko, > Thanks for submitting the patch. The patch looks good for me but I > have some remarks: > - In mac-low.cc the same piece of code replacing station- > >ReportDataFailed() is repeated 3 times using copy & paste, can you > create a small function instead or encapsulate the behavior in > another routine. You can do the same for ReportDataOk, > ReportRtsFailed... ReportDataOk that could be useful in order to avoid future copy and paste. About ReportRtsFailed and ReportCtsOk i think that there is no need to create separate routine because they will use only one time: when we'll receive a Cts and when a CtsTimeout occurs. > -In wifi-remote-station-manager.cc: > -line 811, please do not use NS_ASSERT (false) and use instead > NS_FATAL_ERROR and include a comprehensive message Ok. > -Also the code if (ac == AC_UNDEF) .... is repeated several times > using copy and paste, sometimes with minor changes like > *mqosSsrc.find (ac)->second = 0 instead of ++, please encapsulate > this piece of code into a function with comprehensive name I agree. I'll define two function: WifiRemoteStation::UpdateSsrcCounter (enum AccessClass ac, bool reset) WifiRemoteStation::UpdateSlrcCounter (enum AccessClass ac, bool reset) > -Line 927: //How should we use ac parameter here? I have no answer > for that, do you have one? some plans?? Unfortunately i have no idea. Maybe we should ask Mathieu or other authors of wifi managers. Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
Re: Bug 602 statusIl giorno 23/ott/09, alle ore 12:00, Kirill Andreev ha scritto: > > Hi!, in addition to said above, could you, please, fix this > copy&paste in WifiRemoteStationManager class: > > TracedValue<uint32_t> m_ssrc; > TracedValue<uint32_t> m_slrc; > /* voice access class retry counters */ > TracedValue<uint32_t> m_voSsrc; > TracedValue<uint32_t> m_voSlrc; > /* video access class retry counters */ > TracedValue<uint32_t> m_viSsrc; > TracedValue<uint32_t> m_viSlrc; > /* best-effort access class retry counters */ > TracedValue<uint32_t> m_beSsrc; > TracedValue<uint32_t> m_beSlrc; > /* background access class retry counters */ > TracedValue<uint32_t> m_bkSsrc; > TracedValue<uint32_t> m_bkSlrc;. > > May be is it better to make std::map<AccessClass, uint32_t> and call > traced callback at each change of each retry counter like the > following > TracedCallback<AccessClass, uint32_t> m_slrcCallback; and call > m_slrcCallback(ac, retryCount)? > > Is it worth to replace this huge set of traced values (and trace > sources in GetTypeId) with two maps and two traced callbacks? > It seems to me that fixed number of available access classes is not > good here. access classes. The total number of possible access classes is 8; 9 if we consider AC_BE_NQOS access class. If this number was variable i'd agree with you, but in this condition i think that use of traced callback is tedius. Regards, Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
Re: Bug 602 status (patch)Here is the patch for bug #602 with the corrections that Faker suggested. If there are no objections or advices i'll apply it. Thank you all. Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
|
|
|
Re: Bug 602 status (patch)Hi Mirco & all,
I understand that Bug 602 is an issue and I acknowledge your effort in trying to solve that; however, I am not very convinced about it. I would rather propose to change WifiRemoteStationManager so that it maintains an instance of WifiRemoteStation for every receiver address and access category (tid). This would require the following modifications: - add a new member variable to WifiRemoteStation: uint8_t m_tid; - change this member method of WifiRemoteStationManager WifiRemoteStation *Lookup (Mac48Address address) to the following: WifiRemoteStation *Lookup (Mac48Address address, uint8_t tid) - change the implementation of WifiRemoteStationManager::Lookup() in wifi-remote-station-manager.cc so that it checks for the tid in addition to the remote station address. BTW, I've just been re-reading prior emails on this subject, and I get the impression that this is similar to what Mathieu was proposing. What do you think? Regards, Nicola Kirill Andreev wrote: >> Here is the patch for bug #602 with the corrections that Faker suggested. >> If there are no objections or advices i'll apply it. >> >> Thank you all. >> >> Mirko > > To apply a patch you need one more +1 from Pavel Boyko, but he is away > till monday. > or by Nicola Baldo. > I am doing on behalf of Pavel Boyko because he is away now. > I still do not agree with copy-paste in WifiRemoteStationManager > > Regard, > Kirill Andreev, > IITP RAS |
|
|
Re: Bug 602 status (patch)Il giorno 23/ott/09, alle ore 19:26, Nicola Baldo ha scritto: > Hi Mirco & all, > > I understand that Bug 602 is an issue and I acknowledge your effort > in trying to solve that; however, I am not very convinced about it. > I would rather propose to change WifiRemoteStationManager so that it > maintains an instance of WifiRemoteStation for every receiver > address and access category (tid). This would require the following > modifications: > > - add a new member variable to WifiRemoteStation: > uint8_t m_tid; > > - change this member method of WifiRemoteStationManager > WifiRemoteStation *Lookup (Mac48Address address) > to the following: > WifiRemoteStation *Lookup (Mac48Address address, uint8_t tid) > > - change the implementation of WifiRemoteStationManager::Lookup() in > wifi-remote-station-manager.cc so that it checks for the tid in > addition to the remote station address. > > BTW, I've just been re-reading prior emails on this subject, and I > get the impression that this is similar to what Mathieu was proposing. > > What do you think? advantages in respect to my patch. Moreover, changing signature of WifiRemoteStationManager::Lookup method involves a lot of tedious changes. Regards, Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
Re: Bug 602 status (patch)On Sat, 2009-10-24 at 10:54 +0200, Mirko Banchi wrote:
> > I understand that Bug 602 is an issue and I acknowledge your effort > > in trying to solve that; however, I am not very convinced about it. > > I would rather propose to change WifiRemoteStationManager so that it > > maintains an instance of WifiRemoteStation for every receiver agreed. > > address and access category (tid). This would require the following > > modifications: > > > > - add a new member variable to WifiRemoteStation: > > uint8_t m_tid; > > > > - change this member method of WifiRemoteStationManager > > WifiRemoteStation *Lookup (Mac48Address address) > > to the following: > > WifiRemoteStation *Lookup (Mac48Address address, uint8_t tid) > > > > - change the implementation of WifiRemoteStationManager::Lookup() in > > wifi-remote-station-manager.cc so that it checks for the tid in > > addition to the remote station address. > > > > BTW, I've just been re-reading prior emails on this subject, and I > > get the impression that this is similar to what Mathieu was proposing. > > > > What do you think? > > I think that it could be a solution but i don't understand the > advantages in respect to my patch. Moreover, changing signature of > WifiRemoteStationManager::Lookup method involves a lot of tedious > changes. The compiler will trivially catch all of the needed changes as errors so, it's just automated work: you can switch off your brain and get it done in less than 30mins :) If you need access to a nice build machine, I can give you an account on the 8-core/16GB box rahan.inria.fr. Mathieu |
|
|
Re: Bug 602 status (patch)Il giorno 25/ott/09, alle ore 09:19, Mathieu Lacage ha scritto: > On Sat, 2009-10-24 at 10:54 +0200, Mirko Banchi wrote: > >>> I understand that Bug 602 is an issue and I acknowledge your effort >>> in trying to solve that; however, I am not very convinced about it. >>> I would rather propose to change WifiRemoteStationManager so that it >>> maintains an instance of WifiRemoteStation for every receiver > > agreed. > >>> address and access category (tid). This would require the following >>> modifications: >>> >>> - add a new member variable to WifiRemoteStation: >>> uint8_t m_tid; class. See section 9.9.1.6 in IEEE802.11 standard. Better a variable like the following: AccessClass m_ac. >>> >>> - change this member method of WifiRemoteStationManager >>> WifiRemoteStation *Lookup (Mac48Address address) >>> to the following: >>> WifiRemoteStation *Lookup (Mac48Address address, uint8_t tid) The same of above: WifiRemoteStation *Lookup (Mac48Address address, AccessClass ac) > The compiler will trivially catch all of the needed changes as errors > so, it's just automated work: you can switch off your brain and get it > done in less than 30mins :) > > If you need access to a nice build machine, I can give you an > account on > the 8-core/16GB box rahan.inria.fr. > Ok! If i need it i'll ask you! Thank you. Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
Re: Bug 602 status (patch)Hi Mirko,
>>>> - add a new member variable to WifiRemoteStation: >>>> uint8_t m_tid; > > This doens't work. :( The counter must be mainteined for each access > class. See section 9.9.1.6 in IEEE802.11 standard. > Better a variable like the following: > > AccessClass m_ac. > [...] >>>> - change this member method of WifiRemoteStationManager >>>> WifiRemoteStation *Lookup (Mac48Address address) >>>> to the following: >>>> WifiRemoteStation *Lookup (Mac48Address address, uint8_t tid) > > The same of above: > > WifiRemoteStation *Lookup (Mac48Address address, AccessClass ac) > I think the two solutions (AC vs. TID) are equivalent since there is a one-to-one mapping from a TID of an outgoing frame to an AC. However, after reading section 9.9.1.6 in the standard I agree that using AC is more appropriate, so I am fine with the above changes that you proposed. Nicola |
|
|
Re: Bug 602 status (patch)Il giorno 26/ott/09, alle ore 10:16, Nicola Baldo ha scritto: > Hi Mirko, > > >>>>> - add a new member variable to WifiRemoteStation: >>>>> uint8_t m_tid; >> This doens't work. :( The counter must be mainteined for each >> access class. See section 9.9.1.6 in IEEE802.11 standard. >> Better a variable like the following: >> AccessClass m_ac. > [...] >>>>> - change this member method of WifiRemoteStationManager >>>>> WifiRemoteStation *Lookup (Mac48Address address) >>>>> to the following: >>>>> WifiRemoteStation *Lookup (Mac48Address address, uint8_t tid) >> The same of above: >> WifiRemoteStation *Lookup (Mac48Address address, AccessClass ac) > > I think the two solutions (AC vs. TID) are equivalent since there is > a one-to-one mapping from a TID of an outgoing frame to an AC. > However, after reading section 9.9.1.6 in the standard I agree that > using AC is more appropriate, so I am fine with the above changes > that you proposed. WifiRemoteStation with different TIDs that are mapped to the same AC have to share the same counter. Regards, Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
Re: Bug 602 status (patch)Hi Mathieu,Nicola and all, i'm analysing your solution for correction of bug 602. I noticed we have to also change signature of MacLow::GetStation method: MacLow::GetStation (Mac48Address, const WifiMacHeader&) That involves other changes to other functions like: MacLow::GetAckDuration MacLow::GetAckTxModeForData MacLow::GetCtsTxModeForRts .... However all functions that call MacLow::GetStation. What do you think? Regards, Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
Re: Bug 602 status (patch)On Mon, 2009-10-26 at 15:41 +0100, Mirko Banchi wrote:
> That involves other changes to other functions like: > > MacLow::GetAckDuration > MacLow::GetAckTxModeForData > MacLow::GetCtsTxModeForRts > .... > > However all functions that call MacLow::GetStation. > > What do you think? It's not clear to me what the best solution is but it seems to me that if you did what nicola suggested first, that is, use the tid instead of the AC within the Lookup method signature, you could perform the necessary conversion from tid to AC within Lookup itself instead of requiring the caller to do it and make MacLow::GetStation take a tid as second argument which should not be too hard. Mathieu |
|
|
Re: Bug 602 status (patch)Hi Mirko & Mathieu,
Mathieu Lacage wrote: > On Mon, 2009-10-26 at 15:41 +0100, Mirko Banchi wrote: > >> That involves other changes to other functions like: >> >> MacLow::GetAckDuration >> MacLow::GetAckTxModeForData >> MacLow::GetCtsTxModeForRts >> .... >> >> However all functions that call MacLow::GetStation. >> >> What do you think? > > It's not clear to me what the best solution is but it seems to me that > if you did what nicola suggested first, that is, use the tid instead of > the AC within the Lookup method signature, you could perform the > necessary conversion from tid to AC within Lookup itself instead of > requiring the caller to do it and make MacLow::GetStation take a tid as > second argument which should not be too hard. > Sorry I forgot to state clearly that I agree with Mirko's last argument on TID vs AC, i.e., that since different TIDs can be mapped to the same AC the two solutions are not always equivalent, and we need to use AC to comply with Section 9.9.1.6 in the standard in all possible cases. So the current "most agreed" modification is this one: WifiRemoteStation * WifiRemoteStationManager::Lookup (Mac48Address address, AccessClass ac); As for the changes in the signature of MacLow::GetStation, what about doing the following? WifiRemoteStation * MacLow::GetStation (const WifiMacHeader& hdr) const { return m_stationManager->Lookup (hdr.GetAddr2 (), QosUtilsMapTidToAc (hdr.GetQosTid ())); } this means that in mac-low.cc all current calls of the type GetStation (m_currentHdr.GetAddr1 ()); would be simply changed into GetStation (m_currentHdr); Regards, Nicola |
|
|
Re: Bug 602 status (patch)Il giorno 30/ott/09, alle ore 10:43, Nicola Baldo ha scritto: > Hi Mirko & Mathieu, > > Mathieu Lacage wrote: >> On Mon, 2009-10-26 at 15:41 +0100, Mirko Banchi wrote: >>> That involves other changes to other functions like: >>> >>> MacLow::GetAckDuration >>> MacLow::GetAckTxModeForData >>> MacLow::GetCtsTxModeForRts >>> .... >>> >>> However all functions that call MacLow::GetStation. >>> >>> What do you think? >> It's not clear to me what the best solution is but it seems to me >> that >> if you did what nicola suggested first, that is, use the tid >> instead of >> the AC within the Lookup method signature, you could perform the >> necessary conversion from tid to AC within Lookup itself instead of >> requiring the caller to do it and make MacLow::GetStation take a >> tid as >> second argument which should not be too hard. > > Sorry I forgot to state clearly that I agree with Mirko's last > argument on TID vs AC, i.e., that since different TIDs can be mapped > to the same AC the two solutions are not always equivalent, and we > need to use AC to comply with Section 9.9.1.6 in the standard in all > possible cases. > > So the current "most agreed" modification is this one: > > WifiRemoteStation * > WifiRemoteStationManager::Lookup (Mac48Address address, AccessClass > ac); > > > As for the changes in the signature of MacLow::GetStation, what > about doing the following? > > WifiRemoteStation * > MacLow::GetStation (const WifiMacHeader& hdr) const > { > return m_stationManager->Lookup (hdr.GetAddr2 (), > QosUtilsMapTidToAc (hdr.GetQosTid ())); > } > > this means that in mac-low.cc all current calls of the type > GetStation (m_currentHdr.GetAddr1 ()); > would be simply changed into > GetStation (m_currentHdr); > type (addr1 or addr2) for GetStation method. However the problems are in MacLow. For instance, how would you call GetStation in MacLow::GetCtsTxModeForRts? I think that in some cases this solution is not applicable. In addiction, i think that there is also another bug in MacLow. Function like NormalAckTimeout, FastAckTimeout... call method GetStation with argument m_currentHdr.GetAddr1 () but should it be m_currentHdr.GetAddr2 () instead? Tx station should increment its retry counter not Rx station. What do you think? best regards, Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
|
|
Re: Bug 602 status (patch)Hi,
>> this means that in mac-low.cc all current calls of the type >> GetStation (m_currentHdr.GetAddr1 ()); >> would be simply changed into >> GetStation (m_currentHdr); >> > > I think taht with this change it''s not possible to choose address type > (addr1 or addr2) for GetStation method. Ok so that was just a stupid idea, let's forget it. Sorry for the noise. However the problems are in MacLow. > For instance, how would you call GetStation in > MacLow::GetCtsTxModeForRts? I think that in some cases this solution is > not applicable. So what you say is that when a STA receives a RTS it needs to call GetStation (), but since the TID is not provided within the RTS we cannot determine the AC and pass it to GetStation (). I agree that's a problem. I think that more in general the issue is that WifiRemoteStation handles many different functionalities which are related to a connection (a tx-rx pair), however the exact definition of connection varies slightly for each functionality considered. Examples: retx count -> connection identified by remote address and AC STA Association -> connection identified by desination address only? Rate Adaptation -> standard doesn't define it, but it would be a nice feature if a connection were identified by remote address and AC or TID At this point I don't know what could be a good solution to this problem. Any ideas? Nicola |
|
|
Re: Bug 602 status (patch)Il giorno 30/ott/09, alle ore 12:31, Nicola Baldo ha scritto: > Hi, > >>> this means that in mac-low.cc all current calls of the type >>> GetStation (m_currentHdr.GetAddr1 ()); >>> would be simply changed into >>> GetStation (m_currentHdr); >>> >> I think taht with this change it''s not possible to choose address >> type (addr1 or addr2) for GetStation method. > > Ok so that was just a stupid idea, let's forget it. Sorry for the > noise. > > However the problems are in MacLow. >> For instance, how would you call GetStation in >> MacLow::GetCtsTxModeForRts? I think that in some cases this >> solution is not applicable. > > > So what you say is that when a STA receives a RTS it needs to call > GetStation (), but since the TID is not provided within the RTS we > cannot determine the AC and pass it to GetStation (). I agree that's > a problem. > > I think that more in general the issue is that WifiRemoteStation > handles many different functionalities which are related to a > connection (a tx-rx pair), however the exact definition of > connection varies slightly for each functionality considered. > Examples: > > retx count -> connection identified by remote address and AC > STA Association -> connection identified by desination address only? > Rate Adaptation -> standard doesn't define it, but it would be a > nice feature if a connection were identified by remote address and > AC or TID > > At this point I don't know what could be a good solution to this > problem. Any ideas? > access class information is passed to methods WifiRemoteStation::ReportDataFailed, WifiRemoteStation::ReportRtsFailed... directly with the call. The patch is in attachment. Please take a look at it. . What do you think about above-mentioned bug in MacLow::NormalAckTimeout, MacLow::FastAckTimeout ...? Thank you, Mirko -- Mirko Banchi e-mail: mk.banchi@... e-mail: mk.banchi@... id-jabber: mk.banchi@... PGP key fingerprint: 308F BFB1 4E67 2522 C88E DC69 7631 52ED 32A5 6456 |
| < Prev | 1 - 2 | Next > |
| Free embeddable forum powered by Nabble | Forum Help |