Hi Belgin, would you be able to make a new version of this patch within this week? We are hoping to make a release candidate for Replicant 6.0004 next Monday and release the following Sunday.
Joonas Denis 'GNUtoo' Carikli: > Hi, > > I'm really sorry for the delay. > > Thanks a lot for working on adding native WiFi support for external > dongles. > > I've some comments on the patch below, in order to improve it (and send > a second version when this is done). > >> Native Android WiFi with external dongle > Maybe something like would be better: > 'Add native Android support for external WiFi dongles' > > On Fri, 30 Aug 2019 18:26:58 +0000 > Belgin ?tirbu <belginsti...@hotmail.com> wrote: > >> This patch allows choosing to use the proprietary firmware blob and >> internal WiFi device if the proprietary firmware blob is found. >> If proprietary firmware is not found, the patch causes Android's >> WiFi State Machine to try to use the external dongle. > I think that the 'blob' work is unnecessary. > Something like that could be better: >> With this patch, if the nonfree firmware is found, the internal WiFi >> is used, otherwise the WiFi state machine tries to use the external >> WiFi adapter. > >> The problem with this patch is that Android's WiFi State Machine >> tries to enable Bluetooth and IP filters, causing errors in >> wpa_supplicant, and after 5 errors, wpa_supplicant thinks >> the driver hanged so it disconnects. The workaround to this >> is to add a boolean in the WifiStateMachine class that >> represents if the connection is via dongle, and avoid >> enabling Bluetooth and IP filters in case it is. > Here it doesn't try to enable Bluetooth. Instead it deactivates and > activate back a (Linux?) feature called Bluetooth Coexistence. > > Hardware is complicated... > > The issue is that WiFi and Bluetooth can use the same radio frequencies > (2.4GHz) and probably sometimes even the same antenna. > > So if Both the WiFi and Bluetooth try to transmit at the same time, > they would interfere with each other, and the resulting signal being > transmitted would be of very bad quality. > > So in Linux there is a mechanism to deal with that that is called > Bluetooth coexistance. There is more background information on it here: > https://wireless.kernel.org/en/users/Documentation/Bluetooth-coexistence > > As for why it's enabled I've seen this commit inside the same > repository (frameworks/opt/net/wifi): >> commit 550b2696366dcad08c392c3617a18f70b6e7edf0 >> Author: Nalla Kartheek <kar...@codeaurora.org> >> Date: Mon Jul 27 12:48:39 2015 +0530 >> >> Wi-Fi: Set BTCOEXMODE_DISABLED irrespective of BT's connection >> state >> BTCOEXMODE_DISABLE has to be triggered during the DHCP phase even >> on an active BT connection to ensure that Wi-Fi is given >> preference over the BT. >> This commit ensures the same >> >> Change-Id: I000a79cc3f13c2a91287386558294f120ff54e78 > However I am not familiar enough with the Linux WiFi stack to have more > insights than what the commit says. > >> This is a workaround because the dongle might actually have >> Bluetooth, but I don't know what a proper fix to this would be. > Do you have a log of the errors you just mentioned above? > > I've looked at 5.3, and the ath9k_htc driver has a btcoex_enable option > in its module. However I failed to figure out what was the > default (in a reasonable time) in the code as it's not set, and > module_param_named ends up using some compiler/linker annotations: >> __attribute__ ((unused,__section__ >> ("__param"),aligned(sizeof(void *)))) \ >> = { __param_str_##name, THIS_MODULE, ops, >> \ VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } } > > But if I load the ath9k_htc module on 5.2, the default seem to be 0. > > So we might also be able to workaround the issue by setting some kernel > parameter like ath9k_htc.btcoex_enable=1 > >> Signed-off-by: belginstirbu <belginsti...@hotmail.com> >> --- >> .../com/android/server/wifi/WifiStateMachine.java | 134 >> +++++++++++++-------- 1 file changed, 87 insertions(+), 47 >> deletions(-) >> >> diff --git >> a/service/java/com/android/server/wifi/WifiStateMachine.java >> b/service/java/com/android/server/wifi/WifiStateMachine.java index >> 38d0ac8..2acda79 100644 --- >> a/service/java/com/android/server/wifi/WifiStateMachine.java +++ >> b/service/java/com/android/server/wifi/WifiStateMachine.java @@ >> -198,7 +198,8 @@ public class WifiStateMachine extends StateMachine >> implements WifiNative.WifiPno private ConnectivityManager mCm; >> private DummyWifiLogger mWifiLogger; private WifiApConfigStore >> mWifiApConfigStore; >> - private final boolean mP2pSupported; >> + private boolean mP2pSupported; >> + private boolean mDongleConnected; >> private boolean mIbssSupported; >> private final AtomicBoolean mP2pConnected = new >> AtomicBoolean(false); private boolean mTemporarilyDisconnectWifi = >> false; @@ -1155,6 +1156,8 @@ public class WifiStateMachine extends >> StateMachine implements WifiNative.WifiPno mP2pSupported = >> mContext.getPackageManager().hasSystemFeature( >> PackageManager.FEATURE_WIFI_DIRECT); >> + mDongleConnected = false; >> + >> mWifiNative = new WifiNative(mInterfaceName); >> mWifiConfigStore = new WifiConfigStore(context,this, >> mWifiNative); mWifiAutoJoinController = new >> WifiAutoJoinController(context, this, @@ -5262,8 +5265,8 @@ public >> class WifiStateMachine extends StateMachine implements >> WifiNative.WifiPno private void >> handleSupplicantConnectionLoss(boolean killSupplicant) { /* Socket >> connection can be lost when we do a graceful shutdown > > >> - * or when the driver is hung. Ensure supplicant is stopped >> here. >> - */ >> + * or when the driver is hung. Ensure supplicant is stopped >> here. >> + */ > The lines look identical but they are not. > Previously there was 8 spaces before the '*' which was changed by a Tab > and a space. Replacing that with 8 spaces should make that change > disappear from the diff. > >> if (killSupplicant) { >> mWifiMonitor.killSupplicant(mP2pSupported); >> } >> @@ -5274,27 +5277,31 @@ public class WifiStateMachine extends >> StateMachine implements WifiNative.WifiPno >> void handlePreDhcpSetup() { >> mDhcpActive = true; >> - // Disable the coexistence mode >> - mWifiNative.setBluetoothCoexistenceMode( >> + >> + if(false == mDongleConnected) > Using 'if(mDongleConnected == false)' instead should look better as it > is closer to English. > >> + { >> + // Disable the coexistence mode >> + mWifiNative.setBluetoothCoexistenceMode( >> mWifiNative.BLUETOOTH_COEXISTENCE_MODE_DISABLED); >> >> - // Disable power save and suspend optimizations during DHCP >> - // Note: The order here is important for now. Brcm driver >> changes >> - // power settings when we control suspend mode optimizations. >> - // TODO: Remove this comment when the driver is fixed. >> - setSuspendOptimizationsNative(SUSPEND_DUE_TO_DHCP, false); >> - mWifiNative.setPowerSave(false); >> + // Disable power save and suspend optimizations during >> DHCP >> + // Note: The order here is important for now. Brcm >> driver changes >> + // power settings when we control suspend mode >> optimizations. >> + // TODO: Remove this comment when the driver is fixed. >> + setSuspendOptimizationsNative(SUSPEND_DUE_TO_DHCP, >> false); >> + mWifiNative.setPowerSave(false); >> + } >> >> - // Update link layer stats >> - getWifiLinkLayerStats(false); >> + // Update link layer stats >> + getWifiLinkLayerStats(false); > Here the lines also look identical. Replacing the tab that is at the > beginning of the line by 8 spaces should remove that from the diff. > >> - /* P2p discovery breaks dhcp, shut it down in order to get >> through this */ >> - Message msg = new Message(); >> - msg.what = WifiP2pServiceImpl.BLOCK_DISCOVERY; >> - msg.arg1 = WifiP2pServiceImpl.ENABLED; >> - msg.arg2 = DhcpStateMachine.CMD_PRE_DHCP_ACTION_COMPLETE; >> - msg.obj = mDhcpStateMachine; >> - mWifiP2pChannel.sendMessage(msg); >> + /* P2p discovery breaks dhcp, shut it down in order to get >> through this */ >> + Message msg = new Message(); >> + msg.what = WifiP2pServiceImpl.BLOCK_DISCOVERY; >> + msg.arg1 = WifiP2pServiceImpl.ENABLED; >> + msg.arg2 = DhcpStateMachine.CMD_PRE_DHCP_ACTION_COMPLETE; >> + msg.obj = mDhcpStateMachine; >> + mWifiP2pChannel.sendMessage(msg); >> } > Here the lines also look identical. Replacing the tab that is at the > beginning of the line by 8 spaces should remove that from the diff. > >> @@ -5337,15 +5344,18 @@ public class WifiStateMachine extends >> StateMachine implements WifiNative.WifiPno } >> >> void handlePostDhcpSetup() { >> - /* Restore power save and suspend optimizations */ >> - setSuspendOptimizationsNative(SUSPEND_DUE_TO_DHCP, true); >> - mWifiNative.setPowerSave(true); >> + if(false == mDongleConnected) > Using 'if(mDongleConnected == false)' instead should look better as it > is closer to English. > >> + { >> + /* Restore power save and suspend optimizations */ >> + setSuspendOptimizationsNative(SUSPEND_DUE_TO_DHCP, true); >> + mWifiNative.setPowerSave(true); >> >> - >> mWifiP2pChannel.sendMessage(WifiP2pServiceImpl.BLOCK_DISCOVERY, >> WifiP2pServiceImpl.DISABLED); >> + >> mWifiP2pChannel.sendMessage(WifiP2pServiceImpl.BLOCK_DISCOVERY, >> WifiP2pServiceImpl.DISABLED); >> - // Set the coexistence mode back to its default value >> - mWifiNative.setBluetoothCoexistenceMode( >> + // Set the coexistence mode back to its default value >> + mWifiNative.setBluetoothCoexistenceMode( >> mWifiNative.BLUETOOTH_COEXISTENCE_MODE_SENSE); >> + } >> >> mDhcpActive = false; >> } >> @@ -6047,7 +6057,13 @@ public class WifiStateMachine extends >> StateMachine implements WifiNative.WifiPno >> * Avoids issues with drivers that do not handle >> interface down >> * on a running supplicant properly. >> */ >> + >> + mDongleConnected = false; >> + > The like above shows in red in 'git show HEAD' because it has no text > but there is invisible spaces and tabs. Removing the invisible spaces > and tab fixes that. > >> mWifiMonitor.killSupplicant(mP2pSupported); >> + > Same here: There are also invisible spaces and tabs to remove. > >> + mP2pSupported = >> mContext.getPackageManager().hasSystemFeature( >> + PackageManager.FEATURE_WIFI_DIRECT); >> >> if (mWifiNative.loadDriver()) { >> try { >> @@ -6096,7 +6112,23 @@ public class WifiStateMachine extends >> StateMachine implements WifiNative.WifiPno } >> } else { >> loge("Failed to load driver"); > Maybe that could be changed to something like: > 'loge("Failed to load native driver");' >> - setWifiState(WifiManager.WIFI_STATE_FAILED); >> + >> + loge("Trying to load external wifi dongle"); > And that could be improved with something like 'Now trying to load > [...]' or 'Trying to load [...] instead'. > >> + >> + mP2pSupported = false; >> + > There is the same above: invisible spaces and tabs to remove. >> + mWifiMonitor.killSupplicant(mP2pSupported); >> + > And again, there is some invisible spaces and tabs to remove. >> + if >> (mWifiNative.startSupplicant(mP2pSupported)) { >> + setWifiState(WIFI_STATE_ENABLING); >> + if (DBG) log("Supplicant start >> successful"); >> + mWifiMonitor.startMonitoring(); >> + mDongleConnected = true; >> + transitionTo(mSupplicantStartingState); >> + } else { >> + loge("Failed to start supplicant!"); >> + >> setWifiState(WifiManager.WIFI_STATE_FAILED); >> + } >> } >> break; >> case CMD_START_AP: >> @@ -6527,24 +6559,28 @@ public class WifiStateMachine extends >> StateMachine implements WifiNative.WifiPno mInDelayedStop = false; >> mDelayedStopCounter++; >> updateBatteryWorkSource(null); >> - /** >> - * Enable bluetooth coexistence scan mode when bluetooth >> connection is active. >> - * When this mode is on, some of the low-level scan >> parameters used by the >> - * driver are changed to reduce interference with >> bluetooth >> - */ >> - >> mWifiNative.setBluetoothCoexistenceScanMode(mBluetoothConnectionActive); >> - /* initialize network state */ >> - setNetworkDetailedState(DetailedState.DISCONNECTED); >> >> - /* Remove any filtering on Multicast v6 at start */ >> - mWifiNative.stopFilteringMulticastV6Packets(); >> - >> - /* Reset Multicast v4 filtering state */ >> - if (mFilteringMulticastV4Packets.get()) { >> - mWifiNative.startFilteringMulticastV4Packets(); >> - } else { >> - mWifiNative.stopFilteringMulticastV4Packets(); >> - } >> + if(false == mDongleConnected) > Using 'if(mDongleConnected == false)' instead should look better as it > is closer to English. >> + { >> + /** >> + * Enable bluetooth coexistence scan mode when >> bluetooth connection is active. >> + * When this mode is on, some of the low-level scan >> parameters used by the >> + * driver are changed to reduce interference with >> bluetooth >> + */ >> + >> mWifiNative.setBluetoothCoexistenceScanMode(mBluetoothConnectionActive); >> + /* initialize network state */ >> + setNetworkDetailedState(DetailedState.DISCONNECTED); >> + >> + /* Remove any filtering on Multicast v6 at start */ >> + mWifiNative.stopFilteringMulticastV6Packets(); >> + >> + /* Reset Multicast v4 filtering state */ >> + if (mFilteringMulticastV4Packets.get()) { >> + mWifiNative.startFilteringMulticastV4Packets(); >> + } else { >> + mWifiNative.stopFilteringMulticastV4Packets(); >> + } >> + } >> >> mDhcpActive = false; >> >> @@ -6647,9 +6683,13 @@ public class WifiStateMachine extends >> StateMachine implements WifiNative.WifiPno } >> break; >> case CMD_BLUETOOTH_ADAPTER_STATE_CHANGE: >> - mBluetoothConnectionActive = (message.arg1 != >> - BluetoothAdapter.STATE_DISCONNECTED); >> - >> mWifiNative.setBluetoothCoexistenceScanMode(mBluetoothConnectionActive); >> + if(false == mDongleConnected) > Using 'if(mDongleConnected == false)' instead should look better as it > is closer to English. >> + { >> + mBluetoothConnectionActive = (message.arg1 != >> + >> BluetoothAdapter.STATE_DISCONNECTED); >> + >> mWifiNative.setBluetoothCoexistenceScanMode(mBluetoothConnectionActive); >> + } >> + > The like above shows in red in 'git show HEAD' because it has no text > but there is invisible spaces and tabs. Removing the invisible spaces > and tab fixes that. > > Thanks a lot for the patch. > > I'm looking forward for a second version with the comments above being > addressed. > > Denis. > > > _______________________________________________ > Replicant mailing list > Replicant@osuosl.org > https://lists.osuosl.org/mailman/listinfo/replicant > _______________________________________________ Replicant mailing list Replicant@osuosl.org https://lists.osuosl.org/mailman/listinfo/replicant
