Pulseaudio parseVolume regex fixes #2297

jimduchek · 2017-05-24T18:26:40Z

Specifically, this patch was created to add '.' to the 'dB' part of the regex. However, I noticed two other problems and fixed those as well -

1> monitor_of points to the sink ID that the monitor is... monitoring. For some reason parseVolume() was being called on this, probably a copy-paste error.
2> parseVolume only supported stereo sinks. While this is a common use-case, it's hardly the only one. Now supports 'n' channels. Combined with an update to a newer version of PulseAudio, this ought to fix the original bug reporters issue.

mention-bot · 2017-05-24T18:26:42Z

@jimduchek, thanks for your PR! By analyzing the history of the files in this pull request, we identified @peuter, @kaikreuzer and @martinvw to be potential reviewers.

peuter · 2017-05-24T18:48:47Z

...hab.binding.pulseaudio/src/main/java/org/openhab/binding/pulseaudio/internal/cli/Parser.java

- if (matcher.find()) {
- return Math.round((Integer.valueOf(matcher.group(3)) + Integer.valueOf(matcher.group(7))) / 2);
+ int volumeTotal = 0, nChannels = 0;
+ for (String channel : vol.split(",")) {


This won't work mith my setup, because the volume string here is:
front-left: 30419 / 46% / -20,00 dB, front-right: 30419 / 46% / -20,00 dB
so splitting by "," is not a good idea.

Bah. The ',' in the dB for y'all was precisely why I made this patch in the first place, and then I go and straight up forget about it! :) I will commit a change to ", " instead.

peuter · 2017-05-28T10:53:34Z

...hab.binding.pulseaudio/src/main/java/org/openhab/binding/pulseaudio/internal/cli/Parser.java

@@ -35,8 +35,7 @@
 private static final Logger logger = LoggerFactory.getLogger(Parser.class);

 private static final Pattern pattern = Pattern.compile("^\\s+([a-z\\s._]+)[:=]\\s*<?\"?([^>\"]+)\"?>?$");
- private static final Pattern volumePattern = Pattern.compile(
- "^(0|front-left):( *[0-9]+ \\/)? *([0-9]+)% *\\/? *([0-9\\-, dB]+)?(1|front-right):( *[0-9]+ \\/)? *([0-9]+)% *\\/? *([0-9\\-, dB]+)?$");
+ private static final Pattern volumePattern = Pattern.compile("^([\\w\\-]+):( *[\\d]+ \\/)? *([\\d]+)% *\\/? *([\\d\\-., dB]+)?$");


As the pulseaudio strings that are parsed by this binding are rather diverse regarding spaces, newlines etc. I would prefer a minimalistic change to the regex by just adding \. which was the initial problem that should be solved here, right. It's just a matter of the lessons I have learned during the past years with different pulseaudio versions, that make me a little bit careful when it comes to changes to the regular expressions.

Understood. This change should actually be more permissive than just adding . would be though, and my specific use case does involve a 7.1 surround card (although I can live with support for just stereo channels...). Is there any specific reason why the cli interface was chosen, instead of the native tcp interface? It seems to me the cli interface is almost certainly going to get easily broken by upstream.

CLI vs. TCP:
Well its just easier to use. At the time I wrote the first version of this binding the CLI interface was much easier to use and did everything I needed (and that doesn't seem to have changed yet). I still don't know or understand if and how I can achieve the same things with the native interface.

I always had the little hope that some time someone with more experiance in pulseaudio, would re-write this binding to "do it right" but that never happened. So if you are the one, feel free to start ;-)

Regex change:
Ok you have convinced me, I havn't thought about the multichannel stuff, so your solution should work in more environments that the old one.

I need to do a bit more research on how the native interface works -- it passes structs directly, as far as I can tell. I would hardly call myself any sort of pulseaudio expert, and I'm not sure I have the time on my hands for a rewrite, but I do get a little nervous about parsing/using an interface intended for human use when a 'machine' one is available.

peuter

LGTM

martinvw · 2017-05-29T15:59:03Z

...hab.binding.pulseaudio/src/main/java/org/openhab/binding/pulseaudio/internal/cli/Parser.java

@@ -265,7 +264,7 @@
 source.setVolume(Integer.valueOf(parseVolume(properties.get("volume"))));
 }
 if (properties.containsKey("monitor_of")) {
- source.setMonitorOf(client.getSink(Integer.valueOf(parseVolume(properties.get("monitor_of")))));
+ source.setMonitorOf(client.getSink(Integer.valueOf(properties.get("monitor_of"))));


But is it expected that the method is not called from here anymore?

Completely intended change, i see now

martinvw · 2017-05-29T16:00:25Z

...hab.binding.pulseaudio/src/main/java/org/openhab/binding/pulseaudio/internal/cli/Parser.java

- Matcher matcher = volumePattern.matcher(vol);
- if (matcher.find()) {
- return Math.round((Integer.valueOf(matcher.group(3)) + Integer.valueOf(matcher.group(7))) / 2);
+ int volumeTotal = 0, nChannels = 0;


Can you given them a seperate line, one statement one line :-)

martinvw · 2017-05-29T16:40:29Z

Sorry one final request. Can you squash your commits and change commit message to include a Signed-off-by (to certify that you wrote it or otherwise have the right to pass it on as an open-source), see http://docs.openhab.org/developers/contributing/contributing.html#sign-your-work

For squashing / rebasing see:
http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

Adds support to fetch volume for 1+ channels instead of assuming stereo Fixes bug attempting to use parseVolume to parse monitor_by Signed-off-by: Jim Duchek <jim.duchek@gmail.com> (github: jimduchek)

jimduchek · 2017-05-29T17:04:41Z

Ah done on the rebasing and adding the signed-off-by -- I am not sure where to put the DCO, though. Somewhere here in github, I guess?

martinvw · 2017-05-29T17:06:49Z

By signing off you agree on the global dco, nothing you have to put or upload anywhere.

Adds support to fetch volume for 1+ channels instead of assuming stereo Fixes bug attempting to use parseVolume to parse monitor_by Signed-off-by: Jim Duchek <jim.duchek@gmail.com> (github: jimduchek)

* master: (23 commits) Fixes for setting brightness and colour temperature for V3 white bulbs. (openhab#2338) [RFXCOM] Add new protocols (openhab#2331) [RFXCOM] Add support for UV sensors (openhab#2330) Removed the unsupported MACROMAN encoding (openhab#2332) Create Issue and PR template files (openhab#2285) Corrected the discovery of new devices (openhab#2326) Fixes ./, decimal point issue in volume (openhab#2297) Fix table layout feed binding (openhab#2318) When trying to send a message while the bridge was not connected an NPE occurred (openhab#2299) D-Link Smart Home device binding (openhab#2035) Updated README.md, Changed definition of 'battery_low' channel to sytemwide channel type 'system.low-battery' (openhab#2262) Tesla : Move Event Stream to a separate Thread to speed up processing + discard events that are too old to be relevant + clean-up constants (openhab#2307) Fixed NPE if temperature, powermeter or switch model is null (openhab#1887) [DSCAlarm] Various Fixes and Enhancements (openhab#2313) [globalcache] Implement bi-directional support for serial devices (openhab#2311) Keba : Fix kecontact.xml in ESH-INF (openhab#2314) Keba : Fixes openhab#2237 openhab#2236 openhab#2234 openhab#2038 (openhab#2270) Fixed typo in configuration of channel-type-id (openhab#2310) Update native AllJoyn library for linux x86 and x64 (openhab#2308) [kodi] Added channels for opening PVR TV or Radio streams (openhab#2084) ...

Adds support to fetch volume for 1+ channels instead of assuming stereo Fixes bug attempting to use parseVolume to parse monitor_by Signed-off-by: Jim Duchek <jim.duchek@gmail.com> (github: jimduchek)

…enhab#2297) Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

jimduchek mentioned this pull request May 24, 2017

[pulseaudio] sink volume item state pemanently resets to 0 #1953

Closed

peuter requested changes May 24, 2017

View reviewed changes

peuter reviewed May 28, 2017

View reviewed changes

peuter approved these changes May 29, 2017

View reviewed changes

martinvw reviewed May 29, 2017

View reviewed changes

martinvw requested a review from kaikreuzer May 29, 2017 16:40

Fixes ./, decimal point issue in volume

7018c10

Adds support to fetch volume for 1+ channels instead of assuming stereo Fixes bug attempting to use parseVolume to parse monitor_by Signed-off-by: Jim Duchek <jim.duchek@gmail.com> (github: jimduchek)

martinvw assigned kaikreuzer May 29, 2017

kaikreuzer approved these changes Jun 1, 2017

View reviewed changes

kaikreuzer merged commit 000d4f3 into openhab:master Jun 1, 2017

kaikreuzer modified the milestone: 2.1 Jun 23, 2017

Flole998 pushed a commit to Flole998/openhab-addons that referenced this pull request Dec 30, 2021

Added i18n propertie files for DefaultSystemChannels and Profiles (op…

c9cdf5d

…enhab#2297) Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pulseaudio parseVolume regex fixes #2297

Pulseaudio parseVolume regex fixes #2297

jimduchek commented May 24, 2017

mention-bot commented May 24, 2017

peuter May 24, 2017 •

edited

Loading

jimduchek May 24, 2017

peuter May 28, 2017

jimduchek May 29, 2017

peuter May 29, 2017

jimduchek May 29, 2017

peuter left a comment

martinvw May 29, 2017

martinvw May 29, 2017

martinvw May 29, 2017

martinvw commented May 29, 2017

jimduchek commented May 29, 2017

martinvw commented May 29, 2017 •

edited

Loading

Pulseaudio parseVolume regex fixes #2297

Pulseaudio parseVolume regex fixes #2297

Conversation

jimduchek commented May 24, 2017

mention-bot commented May 24, 2017

peuter May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peuter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinvw commented May 29, 2017

jimduchek commented May 29, 2017

martinvw commented May 29, 2017 • edited Loading

peuter May 24, 2017 •

edited

Loading

martinvw commented May 29, 2017 •

edited

Loading