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

refactor(nm): add NetworkManagerDbusWrapper and ModemManagerDbusWrapper objects #4757

Merged
merged 29 commits into from
Jul 19, 2023

Conversation

mattdibi
Copy link
Contributor

@mattdibi mattdibi commented Jul 11, 2023

This PR adds two new objects to the org.eclipse.kura.nm bundle that abstract the two main services to which the NMDbusConnector interfaces: NetworkManager and ModemManager.

This refactoring was necessary since the NMDbusConnector, initially dedicated to interfacing only with NetworkManager, grew too much and started touching other services (ModemManager, soon WPA Supplicant) as features were added.

The introduction of these two new objects should increase the readability of the code since it is now clear to which dbus service the NMDbusConnector is talking to.

Additional notes:

  • I moved getModemDeviceHwPath method from NMStatusConverter to the ModemManager wrapper.

Things I don't particularly like:

  • The NetworkManagerDbusWrapper and the ModemManagerDbusWrapper have a dbusConnection as private member, but the sole owner of the dbusConnection is NMDbusConnector (which is why we modeled it as a singleton). Another way to avoid confusion would be to always pass the dbusConnection as parameter of the method. Any suggestion is welcome.
  • The modem reset handlers were moved inside ModemManagerDbusWrapper since ModemManager is responsible for managing the Modem but to decide whether we need to reset the modem or not we monitor a NetworkManager signal. The reset handlers are a "bridge" between the two services but I decided to move them in the ModemManager anyway.
  • Lack of unit tests for the wrappers object. Coverage is pretty high because we have the NMDbusConnector tests but a set of dedicated tests for the wrappers would be a great idea.

Test performed on Raspberry Pi 4:

  • Installation and check that all connection work as expected
  • Wifi connection
  • Cellular connection
  • Modem reset
  • Configuration enforcement
  • Manual IP configuration for ethernet device

Any additional test is welcome

mattdibi added 29 commits July 11, 2023 10:00
@mattdibi mattdibi marked this pull request as ready for review July 18, 2023 07:38
@mattdibi mattdibi requested a review from nicolatimeus July 18, 2023 14:56
@mattdibi mattdibi merged commit 7fe286d into eclipse-kura:develop Jul 19, 2023
@mattdibi mattdibi deleted the refactor/dbus_conn_wrappers branch July 19, 2023 08:20
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.

2 participants