Fix misleading "unknown device type" log when a plugin factory throws - #1453
Open
ngenovese11 with Copilot wants to merge 3 commits into
Open
Fix misleading "unknown device type" log when a plugin factory throws#1453ngenovese11 with Copilot wants to merge 3 commits into
ngenovese11 with Copilot wants to merge 3 commits into
Conversation
Copilot
AI
changed the title
[WIP] Fix misleading log message when device fails to load
Fix misleading "unknown device type" log when a plugin factory throws
Jul 28, 2026
ngenovese11
marked this pull request as ready for review
July 28, 2026 17:35
Contributor
|
@copilot we need to add an empty commit with a semantic label to trigger a new build |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a misleading “unknown device type” error by changing DeviceFactory.GetDevice so that exceptions thrown by a registered plugin factory are no longer swallowed and converted into a null result (which callers previously interpreted as “type not registered”).
Changes:
- Refactors
DeviceFactory.GetDeviceto only returnnullfor “type not found” and allow factory exceptions to propagate to callers. - Updates XML documentation to describe the new behavior.
- Removes unused locals left from the refactor.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+203
to
+207
| var properties = localDc.Properties; | ||
|
|
||
| if (!FactoryMethods.TryGetValue(typeName, out var wrapper)) | ||
| { | ||
| Debug.LogWarning("Device type '{typeName}' not found in DeviceFactory", typeName); | ||
| return null; | ||
| } | ||
| var typeName = localDc.Type.ToLower(); | ||
|
|
||
| Debug.LogInformation("Loading '{type}' from {assemblyName}", typeName, wrapper.Type.Assembly.FullName); | ||
| if (properties is JObject jObject) |
| return null; | ||
| } | ||
|
|
||
| Debug.LogInformation("Loading '{type}' from {assemblyName}", typeName, wrapper.Type.Assembly.FullName); |
Comment on lines
195
to
+198
| /// <returns>An instance of a device that implements <see cref="IKeyed"/>, or <see langword="null"/> if the device type is | ||
| /// not recognized or an error occurs during creation.</returns> | ||
| /// not recognized.</returns> | ||
| /// <exception cref="Exception">Thrown when the registered factory method for the device type throws an exception while creating the | ||
| /// device. Callers should catch and log this exception to report the actual cause of the failure.</exception> |
Contributor
Author
Added an empty |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a plugin's device factory method threw an exception during construction,
DeviceFactory.GetDevicecaught it, logged it, and returnednull. Thenullresult was then misinterpreted by callers as an unregistered device type, producing a misleading"ERROR: Cannot load unknown device type..."message that obscured the real failure cause.Root cause
DeviceFactory.GetDevicewrapped the entire method body (type lookup + factory invocation) in a single try/catch, collapsing "type not found" and "factory threw" into the samenullreturn path.ControlSystem.LoadDevices,LoadRooms) can't distinguish the two cases from anullreturn, so they always assumed "unknown type."Changes
DeviceFactory.GetDevice: no longer catches exceptions from the registered factory method's invocation. Type-not-found still returnsnullwith an accurate warning; any exception fromwrapper.FactoryMethod(localDc)now propagates to the caller.ControlSystem.LoadDevices/LoadRoomsalready hadcatch (Exception e)blocks that log the exception with its message and stack trace viaDebug.LogMessage(e, ...)— this path now actually gets exercised for factory failures instead of being masked.key,name,type) left over from the refactor.Effect