[chromecast] Do not cast the background discovery setting to String

[jahubba]

The problem

ChromecastDiscoveryParticipant.activateOrModifyService() casts the background discovery property to String:

String autoDiscoveryPropertyValue = (String) properties
        .get(DiscoveryService.CONFIG_PROPERTY_BACKGROUND_DISCOVERY);

The value arrives as a Boolean when the setting is written through the REST API (PUT /rest/services/discovery.chromecast/config with {"background": false}), so @Modified throws ClassCastException. SCR logs

[ERROR] bundle org.openhab.binding.chromecast:5.1.1 (275)[...ChromecastDiscoveryParticipant(352)] :
        The modified method has thrown an exception
        at ...ChromecastDiscoveryParticipant.activateOrModifyService(ChromecastDiscoveryParticipant.java:72)
        at ...ChromecastDiscoveryParticipant.modified(ChromecastDiscoveryParticipant.java:66)

and the component keeps its previous value. The result is that turning background discovery off appears to succeed — the config reads back as false — while discovery in fact carries on running. Writing the value as the string "false" does work, which is not discoverable from the outside.

The change

Accept either representation instead of casting.

[Copilot]

Pull request overview

Fixes Chromecast background discovery configuration handling so updates coming from JSON-typed sources (notably the REST API) no longer throw ClassCastException during @Modified, ensuring the updated setting actually takes effect.

Changes:

• Stop casting DiscoveryService.CONFIG_PROPERTY_BACKGROUND_DISCOVERY to String.
• Accept the property as either a Boolean or a non-blank string representation and apply it to isAutoDiscoveryEnabled.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wborn]

Thanks for tracking this down. Accepting the configuration property as either a Boolean or a String fixes the reported ClassCastException and preserves compatibility with the existing string representation.

I found one remaining concurrency issue in the runtime update path. The configuration flag is written by the DS @Modified callback and read from mDNS discovery callbacks running on other threads, but the field is not safely published. I left an inline comment suggesting that it be made volatile.

[wborn]

Thanks, the updated implementation addresses my previous comment. Making isAutoDiscoveryEnabled volatile safely publishes configuration changes to the discovery threads, and the revised conversion correctly supports both Boolean and string configuration values.

I do not see any remaining issues based on this review, but the code should still be inspected manually by me or another reviewer before final approval.

[lsiepel]

I do not see any remaining issues based on this review, but the code should still be inspected manually by me or another reviewer before final approval.

Correction; your AI agent does not see remaining issues. Otherwise you would already have inspected it to come to this conclusion.
I think these types of comments should be clear about being AI generated.
The use of I claims it is personal.

[wborn]

That is a fair point. There are certainly improvements to be made in how I use AI for reviews. Like everyone else, I am also still learning how best to work with it. 😉

I want to provide the agent with better and more reusable context, so I do not have to repeat the same instructions in every prompt and can more consistently verify its findings before submitting a review.

Perhaps the lessons learned could be captured in "AGENTS.md". That would make the guidance reusable by other maintainers and by automated tooling such as Copilot reviews. Improving that file could also help AI-assisted contributors create PRs that better follow the project’s conventions, reducing the number of review comments needed.

At least in my experience, Copilot’s built-in reviews are still fairly limited compared with what can be achieved using models that are given richer project context and more time to reason about the implementation. Good project-specific guidance could benefit both approaches.

In the longer term, we could perhaps use this guidance as the basis for a dedicated AI review bot funded by the openHAB Foundation (CC @kaikreuzer). That would make the process more consistent and transparent, while keeping the final judgement and responsibility with the maintainers.

[lsiepel]

That is a fair point. There are certainly improvements to be made in how I use AI for reviews. Like everyone else, I am also still learning how best to work with it. 😉

JFTR, my comment is not personal, it is part of the whole how and how not to use AI, and that comes with constructive feedback. You don’t seem to take it personal, but I want to state it explicitly.

I want to provide the agent with better and more reusable context, so I do not have to repeat the same instructions in every prompt and can more consistently verify its findings before submitting a review.

I second that and will also open a PR towards "AGENTS.md" to improve. Prompts do change quickly, so hope we can keep up with it.

In the longer term, we could perhaps use this guidance as the basis for a dedicated AI review bot funded by the openHAB Foundation (CC @kaikreuzer). That would make the process more consistent and transparent, while keeping the final judgement and responsibility with the maintainers.

Maybe something for a dedicated thread (or part of existing g AI discussion.

[jahubba]

Just a quick not from me as well. @lsiepel you make a good point that it should clarify the llm is doing the work. It's interesting it uses "I", I'm not sure if it is talking about itself in the first person or if it intentionally tries to speak on the persons behalf. Agents.md would help on the contribution side along with the review and an AI policy would probably be good as well.

This is getting off topic, but agent skills could also be useful for contributing, review, and even using openhab. I came across some of these as I was using an agent to reorganize and cleanup my openhab instance, but have some skills for adding, removing, configuring things/items/etc along with creating rules and other activities.

[wborn]

It's really stubborn when I tell it to use AI instead of I. 🙃

[wborn]

AI re-reviewed the current PR head. The previous concurrency concern has been addressed by making isAutoDiscoveryEnabled volatile, and the updated configuration handling correctly supports both the Boolean value supplied through the REST API and the existing String representation.

AI found no further issues in the changed code. A human maintainer review is still required before final approval.

The requested change has been addressed. A human maintainer review is still required before final approval.

[wborn]

But it's getting better already. 😄

[wborn]

I looked into this a bit further. The background property is semantically a boolean, but the actual value can be either a String or a Boolean depending on how the configuration was written.

The original implementation was added together with support for configuring this through runtime.cfg, where scalar values such as discovery.chromecast:background=false are provided as strings. When the same service configuration is updated through the REST API, however, JSON such as {"background": false} results in an actual Boolean. This explains why the original code expected a String and why that assumption does not work for all configuration paths.

This PR also seems to expose a broader legacy pattern in other discovery participants. The same String-based handling currently appears in:

• KodiUpnpDiscoveryParticipant
• HueBridgeUPNPDiscoveryParticipant
• HueBridgeMDNSDiscoveryParticipant
• OnkyoUpnpDiscoveryParticipant
• HueSyncDiscoveryParticipant
• AVMFritzUpnpDiscoveryParticipant
• PioneerAvrDiscoveryParticipant
• FroniusWattpilotMDNSDiscoveryServiceParticipant
• AndroidTVMDNSDiscoveryParticipant
• FireTVStickMDNSDiscoveryParticipant

Those do not necessarily need to be addressed in this PR, but we should probably clean them up separately so they do not have the same issue when their discovery configuration is changed through REST.

[wborn]

Thanks for the fix!