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

Fix sensorsystem issues #267

Merged
merged 7 commits into from
Sep 13, 2024
Merged

Conversation

earocorn
Copy link

This fixes 2 main issues and amends a minor issue:

Configuration of submodules will not save

  • Problem: SensorSystem config does not get updated when subsystem config is updated.
  • Solution: When SensorSystem receives a CONFIG_CHANGED module event, it will update the config with the new subsystem config.

Submodules do not show up when OSH starts

  • Problem: Submodules do not load until initialization of the SensorSystem, so they won't load unless started or module's autostart is enabled.
  • Solution: Load submodules when SensorSystem loads. NOTE Cannot do this in SensorSystem constructor as the new object does not have config to read subsystems from. So, subsystems are loaded when the SensorSystem receives a SensorSystemConfig.

Minor Changes for System driver database

  • Update the SystemFilter to search for withNoParent() systems to enforce parent system managing subsystems
  • Allow submodules to update their driverhandler if needed. doRegisterMember() checks if submodule already has a handler and updates it if so

Copy link
Collaborator

@nickgaray nickgaray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one minor efficiency comment that, though in most cases won't make things faster (a low number of subsystems), in a case where there are a large number of subsystems it may speed things up a bit when handling config changes.

@@ -100,8 +100,7 @@ protected void doFinishRegister(ISystemDriver driver) throws DataStoreException
{
for (var member: ((ISystemGroupDriver<?>)driver).getMembers().values())
{
if (!(member instanceof IModule<?>)) // don't register submodules as they'll register themselves
doRegisterMember(member, driver.getCurrentDescription().getValidTime());
doRegisterMember(member, driver.getCurrentDescription().getValidTime());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to change this here.
If the system members are themselves modules they should register themselves when they are started.
Was this not working?

I'm thinking there might be an issue when the UID of a member does not match the wildcard of the database that handles the parent. In that case the subsystems might end up in a different database than the parent and that's not good. Did you run into this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed that line because you would have to restart all submodules before they are registered to the database. So it is a similar fix to registering the top level modules that were started before the database is created.

Also, members cannot be in a different database than the parent, but I did see this issue where you can register a wildcard to one database, and then register a member of the wildcard to another database, and no error is thrown. This appears to have already been an issue.
image
In the image, I have a database that takes "...system:systems1:*" and then I am allowed to add a different database with "...system:systems1:system2"
I could create an issue for this or if you have advice on how to handle this I could include it in the PR.

earocorn and others added 3 commits August 15, 2024 14:30
Changed from <module name, module> to <module id, module> because problems occurred when modules had default name
@alexrobin alexrobin merged commit 1895e40 into opensensorhub:v2 Sep 13, 2024
1 check passed
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.

3 participants