Updates for final cdsp0.5.0, wip

[HEnquist]

These are the changes needed in the backend to support the new features of camilladsp v0.5.0 (wav-file reading) and the new config validator in pycamilladsp-plot.

[HEnquist]

This seems to be working ok now. It needs the latest pycamilladsp-plot from master, and the frontend from the final050 branch. @JWahle and @bitkeeper, could you try it to make sure I didn't break anything for moOde? It would also benefit from some more testing in general, the new validation is still very new..

[JWahle]

Cool! Since your new validator returns the path in the json config, it should be pretty easy to show validation errors directly on the input field. If you are not currently working on that, I can implement it.

[HEnquist]

Yes that was basically the idea behind including the path. I haven't started working on that yet. If you want to do it, just go ahead! Do you think this will be a quick thing? Right now the previous gui release doesn't work well with the latest camilladsp, so it would be nice to get a new release out soon, even if it's not perfect.

[bitkeeper]

To be sure I'm testing the right combinations (to much software packages ;-), is below correct?:

• camilladsp: used offical released bin from camilladsp-0.5.0-linux-armv7.tar.gz
• camillagui: branch final050, HEAD@7a2850602658442807a5e861838f2d205798868b
• camillagui-backend: branch final050, HEAD@e823d45f56ce20d51c0763d9775f489a2f5eb1b4
• pycamilladsp: master, HEAD@8b64c91411f97d1db9ec5718f40e220ce60f5ec8 (=24 jan 2021)
• pycamilladsp-plot: branch master, HEAD@af3e3bcaf17243d9eec8b81bf8783f84e0183416

with this combination I get:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/dist-packages/aiohttp/web_protocol.py", line 422, in _handle_request
    resp = await self._request_handler(request)
  File "/usr/local/lib/python3.7/dist-packages/aiohttp/web_app.py", line 499, in _handle
    resp = await handler(request)
  File "/opt/camillagui/backend/views.py", line 188, in get_active_config_file
    json_config = new_config_with_relative_filter_paths(get_yaml_as_json(request, config), config_dir)
  File "/opt/camillagui/backend/filemanagement.py", line 75, in get_yaml_as_json
    validator = request["VALIDATOR"]
  File "/usr/local/lib/python3.7/dist-packages/aiohttp/web_request.py", line 272, in __getitem__
    return self._state[key]
KeyError: 'VALIDATOR'

moOde 7,2 is just released. So probably it will take a while for it will end up in moOde. Also requires some changes on moOde to support the wave files and get rid of the current wave file protection/conversion.

[HEnquist]

Those are the correct versions. The validator somehow went missing, strange! Can you check that your main.py of the backend is ok? It should end like this:

camillaconnection = CamillaConnection(config["camilla_host"], config["camilla_port"])
camillavalidator = CamillaValidator()
#camillaconnection.connect()
app["CAMILLA"] = camillaconnection
app["VALIDATOR"] = camillavalidator
web.run_app(app, port=config["port"])

[bitkeeper]

The main.py ends exactly like that .

[HEnquist]

Oops sorry it's a typo on line 75 of filemanagement.py. It should be

request.app["VALIDATOR"]

[JWahle]

Yes that was basically the idea behind including the path. I haven't started working on that yet. If you want to do it, just go ahead! Do you think this will be a quick thing? Right now the previous gui release doesn't work well with the latest camilladsp, so it would be nice to get a new release out soon, even if it's not perfect.

I'll try to get it finished by monday - if this doesn't work out, I'll let you know.

[HEnquist]

Sounds good, then I'll wait for that.

I fixed the request.app typo now.

[JWahle]

@HEnquist I pushed an implementation, that shows validation errors for the devices and filters tabs. I don't think, the mixers tab needs validation, since valid input/output channel numbers are already enforced by the GUI. Not sure, how much validation the pipeline tab needs - I'll think about it this evening.

The select inputs behave a little strange, when they are invalid, since the config value then deviates from the visible selected value. Other than that, I am happy with the rest.

Here are some things, I noticed about the pycdsp validation:

• devices tab only has one error reported
• filters tab has only one error per filter reported
• pipeline tab filter has channel field not validated

[bitkeeper]

@HEnquist fix works great.

Had an issue with my test 64bit float wave files, reported on the camilladsp page. When using a standard 44.1/16 wave everything went ok.

Found two things that can be an issues for the average user:

1 I notice that when you select different IR wave file the channel number is reseted to 0, makes it easy to break unintentionally your config.

2 When older configs (with raw) are now opend in the gui, those seems broken to the user (due the error messages). Despite those configs are still valid to use. No clear indication about what to do to fix it. It would be nice if those could be automatically converted to the new config format.

[HEnquist]

@HEnquist I pushed an implementation, that shows validation errors for the devices and filters tabs. I don't think, the mixers tab needs validation, since valid input/output channel numbers are already enforced by the GUI. Not sure, how much validation the pipeline tab needs - I'll think about it this evening.

Alright! I'll see if I can try it tonight, but afraid it might not happen until tomorrow. The pipeline could check that the number of channels is correct all the way through, and that filters are filtering an existing channel. Apart from that there isn't muchto check.

Here are some things, I noticed about the pycdsp validation:

* devices tab only has one error reported
* filters tab has only one error per filter reported

The validation is done in two steps. First, it checks the names of keys etc, and all the fixed rules (like Q>0) etc. If no errors are found in step 1, then it goes on to do a set of checks of things that depend on some other parameter. These checks need a "good" config, that's why they aren't done if step 1 already found some problem. Could this be the reason you only get one error?

* pipeline tab filter has `channel` field not validated

(I forgot that, will add!) EDIT: I thought I had forgotten that, but it's already included. You mean the gui doesn't report the errors, or that you don't get them from the validator?

[HEnquist]

1 I notice that when you select different IR wave file the channel number is reseted to 0, makes it easy to break unintentionally your config.

Good point. The gui could have a check so that if the coeffs change from one wav to another, the channel is kept.

2 When older configs (with raw) are now opend in the gui, those seems broken to the user (due the error messages). Despite those configs are still valid to use. No clear indication about what to do to fix it. It would be nice if those could be automatically converted to the new config format.

Also a valid point. But adding that would be a fair amount of work for something that everybody only needs to do once. I'm not expecting any new config file format changes anytime soon.

[HEnquist]

The new error highlighting is fantastic! It gets so much easier to use. Only thing missing (that I have found) is to show something on the pipeline tab when there is a pipeline error. Specifically when there is a mismatch in the number of channels somewhere along the pipeline, or when a filter tries to filter a non-existing channel.

Slightly different topic. Each time I see the "invert" icon now I first think "undo". Could we switch it to something that matches invert better? I'm thinking mdiInvertColors or mdiRotateLeft? Or even mdiFlipVertical? I think I have a slight preference for mdiInvertColors, but I can be convinced to change my mind :)

[JWahle]

1 I notice that when you select different IR wave file the channel number is reseted to 0, makes it easy to break unintentionally your config.

The GUI now keeps the value for

• channel, if selecting a different Wav file
• filename, if switching between Raw and Wav

(changes pushed to https://github.com/HEnquist/camillagui/pull/41)

[JWahle]

The new error highlighting is fantastic! It gets so much easier to use. Only thing missing (that I have found) is to show something on the pipeline tab when there is a pipeline error. Specifically when there is a mismatch in the number of channels somewhere along the pipeline, or when a filter tries to filter a non-existing channel.

I started implementing this, but got blocked by https://github.com/HEnquist/pycamilladsp-plot/issues/6. If you resolve that, I will continue.

Slightly different topic. Each time I see the "invert" icon now I first think "undo". Could we switch it to something that matches invert better? I'm thinking mdiInvertColors or mdiRotateLeft? Or even mdiFlipVertical? I think I have a slight preference for mdiInvertColors, but I can be convinced to change my mind :)

I also considered mdiSineWave (2nd from left) and mdiRepeat (3nd from left). Honestly, I don't think any of them fits perfectly. I went through the entire collection on materialdesignicons.com and couldn't find a satisfying one. If you want to change it, please do - I don't have a strong favorite.

[JWahle]

2 When older configs (with raw) are now opend in the gui, those seems broken to the user (due the error messages). Despite those configs are still valid to use. No clear indication about what to do to fix it. It would be nice if those could be automatically converted to the new config format.

I could build a migration function into the backend, that migrates config files before sending them to the GUI (api/getactiveconfigfile). @HEnquist what do you think?

[HEnquist]

Yeah no icon really fits perfectly. How about mdiSwapVerticalVariant or mdiPlusMinusVariant? edit: mdiSwapVerticalBold and mdiNumericNegative1 could also be candidates:

A migration function could be useful. Thinking a little more, it would be very easy to add in the validator. I can just put in a function that it runs before the validation. Right now it would only search for the File/Raw change, but it could be extended in the future if needed. Then the gui just needs to handle the current config format, no need to worry about anything old.

[HEnquist]

I pushed a new version of the validator to pycamilladsp-plot/master, that updates old configs as the first step.

[bitkeeper]

@JWahle the new error feedback/highlighting is indeed a great improvement, especially the indication on the tab is a great addition.

Case not coverd: I have attached am config of a user on the forum, which has invalid yml, but the gui doesn't say something is wrong with it. camilladsp -c indicate something is wrong with it, but also doesn't give a clue that the yml format it self isn't valid:

$ camilladsp -c test_camilladsp -c test.yml
.yml
Config is not valid
Invalid config file!
while parsing a block mapping, did not find expected key at line 117 column 6

The bad config: test_ymlinvalid.zip

[HEnquist]

I updated the validator now to give a simpler and clearer message for yaml errors. The test_ymlinvalid file just gives this now: "YAML syntax error on line 117". The long comments about block mapping this and that weren't really helpful anyway. A syntax error is reported at the root of the config, so the path list is empty.

[JWahle]

I think, mdiPlusMinusVariant fits very well - nice find! I pushed a commit with the new icon to the final050 branch on the frontend.

[JWahle]

I just updated the validation error logic in the GUI, so I think we are finished here. Here are some configs to test the validation errors (one with most possible errors in devices/filters/mixers and one with pipeline errors).

There are still 2 issues in pycamilladsp-plot, that need to be adressed, before this works properly.

[HEnquist]

I made some minor fixes, added the get/set active config scrip option, and added a way to limit the allowed capture/playback device types for the validator. I think this version is done, anything I have missed?

[JWahle]

I would prefer, leaving the commits regarding the set/get active config out of this release - @bitkeeper has not given his feedback and I also have some remarks. Other than that, I think, we are ready.

[HEnquist]

It would be nice to wait for an answer from bitkeeper, but unless it cases trouble when disabled I would like to keep it in. This way we can get some feedback in what do about this in the next release.

[bitkeeper]

Handling links in Windows is indeed a little odd; shortcuts or junctions als require additional win32 python packages deps for the backend.

For the moment with Linux I also prefer the links; it is very clean and self explaining. And the link is also used by alsa_cdsp/camilladsp it self. With moOde the backend+gui is default off. So maybe we should think a little bit longer about it and follow the suggestion of @JWahle to leave it out for now, because before you know it it is out in the field and someone is using it.

[HEnquist]

... leave it out for now, because before you know it it is out in the field and someone is using it.

That was pretty much the idea. Get it out in the field to collect feedback. I think this is a good thing, not a problem.

[JWahle]

... leave it out for now, because before you know it it is out in the field and someone is using it.

That was pretty much the idea. Get it out in the field to collect feedback. I think this is a good thing, not a problem.

As long, as we haven't settled for a final implementation, we might not get much value out of this feedback - and users might not appreciate, that they have to change their config again after the following release. I don't understand, why it is so urgent, to release this feature right now. We could make a smaller release shortly after this one. However, even though I don't know, how many users will actually use this feature, this is probably not a such a big issue. If I was a compiler, this remark would be a warning - not an error ;) So, if you want to release it as it is - go ahead. I don't see any real blockers.

[HEnquist]

I'm sensing some confusion.. which feature are we actually discussing?

[HEnquist]

I changed and simplified it a bit so there is less windows-specific stuff. The try-except already handles the errors that could come, and I added just a log print specific for windows. For reading the symlink there should be no problem. I also added a note in the readme that the script-feature is experimental, should help avoid disappointment if we change that later. Do you still see problems with this?

[JWahle]

Looks good to me, let's go.

[HEnquist]

Ok, great! Then I'll merge and release this as soon as I can.