Clemens wrote:
I'm just objecting to the fact that the function name and ALSA's error string are combined into a string that cannot easily be parsed later.
As Aldrin pointed out, it is not meant to be parsed. I have now provided separate functions for accessing the function names directly.
I'd like to have only the snd_strerror() result as the message. In this case, the function name has to be stored separately if it is needed.
"alsa::pcm::pcm: snd_pcm_open failed: No such file or directory" is useful for debugging, "No such file or directory" is not. Therefore, I don't see why you want only snd_strerror in that message.
While playback will be used more often, I think the difference between playback and capture is so big that we cannot make one of them the default.
I have removed the default arguments, as you requested.
hw_config(snd_pcm_t* pcm): pcm(pcm) { try { current(); } catch (std::runtime_error&) { any(); }
I don't like using exceptions when there's nothing wrong. Besides, getting the current parameters may fail due to other errors (like device unplugged).
It seems like a sensible fallback, anyway. The other option would be to load any() settings always and have the user call current(), if he wants those instead. Would you find this preferable?
My preference would be that the user has to indicate whether he wants to read the current setting or to begin choosing new settings. It is not possible for this constructor to guess what the user wants.
I have looked at the issue again; the current system (load current if possible, any otherwise) is to make it resemble the API that sw_config provides (always loading the current settings). If the user specifically wants current or any settings (and wants an exception if that fails), he may call the any() and current() functions after first constructing the object. If not, he will get the most sensible default behavior.
However, I changed to catch to only match alsa::error (not that it matters, but it's clearer that way).
Some settings need to be loaded by the constructor in order to ensure that the structure is in a consistent state at all times.
ALSA's snd_pcm_hw_params_t is in a consistent state even before initial or current settings are loaded from a PCM device. But then the hw_config object isn't the direct wrapper for snd_pcm_hw_params_t ...
The documentation says that an invalid snd_pcm_hw_params_t is allocated (by snd_pcm_hw_params_malloc). I did not look into ALSA source code to find out whether this means uninitialized memory or just a kind of "none" configuration space that could still be used by the other functions. In the latter case, removing the initialization from hw_config constructor, and having the user explicitly call any() or current(), would be a sensible choice.
In this case it would be nice if this behavior was also clearly documented on the C API.
In our case, a rollback is equivalent to calling snd_pcm_mmap_commit with frames==0.
Ah, didn't think of that. Fixed to work this way, safe commit function provided.
Hmm, it should be possible to commit any number of frames as long as it isn't more than originally requested.
One could try to commit more than the original number with the old wrapper, but this is fixed now (among with the commit function).
After committing, it is indeed not allowed to access the mmap buffer, so this isn't too unexpected.
Just to stay safe, I made it throw std::logic_error if any access is attempted after commit.
BTW: The object should be called mmap_access or something like that.
This differs from the naming convention used in other parts of the wrapper (take a part of the C API function name). It seems pretty obvious what an object called mmap does on a PCM, so I don't quite see why you want another name for it.
As far as I can see, your header tries to do two things: wrapping the ALSA API, and offering an easier-to-use API that is not as low-level as the ALSA API itself. I think these two should be put in separate header files.
I don't see why they couldn't coexist in the same header. C++ programmers are used to some flexibility (things happening automatically, function overloading, default arguments, "OOP" syntax), so it seems normal to offer these in such a wrapper.
However, I don't intend alsa.hpp, as it is now, as an easy solution for the user, but have instead written a separate audio library on top of it, abstracting many of the features of ALSA and also supporting other audio interfaces under the hood.
Anyway, we need more wrappers for all those data structures. I'll see what I can come up with.
True. The problem is that it is a lot of work, even with all those macros. Without helper macros one would need thousands of lines just to wrap all the functions.
Any help is appreciated, of course :)
Aldrin wrote:
I would prefer that the method from the C++ API should appear instead of the internal C API function. The C++ developer is calling the constructor of alsa::pcm, not snd_pcm_open().
I have fixed this. Now it prints the calling function, the C API function and the snd_strerror message. All can be requested as individual strings to order having to parse the formatted message returned by what().
From a OO perspective, I would expect that a *_config object be created from an existing pcm object. Something like:
alsa::pcm pcm;
pcm.get_config().set(FOO).set(BAR);
This does not allow storing the generated configuration, which is a requirement if we want to add some control logic to it instead of just setting things blindly. (remember, hw_config is noncopyable because it depends on a PCM, which is noncopyable)
If there is something that doesn't apply to a pcm instance, the C++ API could provide an instance of a pcm_any object and call that instead:
alsa::pcm_any.get_config().set(FOO).set(BAR);
Even the "any" configuration is always linked to a PCM: int snd_pcm_hw_params_any(snd_pcm_t *pcm, snd_pcm_hw_params_t *params);
A new version is attached.
Changelog for alsa.hpp 0.6: - alsa::mmap completely rewritten - alsa::error now has accessors for function names - ALSA_CHECKED and alsa::error changed to also store the name of the function using the macro - alsa::pcm device and stream default values removed - catching errors only, instead of any runtime_error - added missing *_param operator=(*_param...) - naming style changed: member variables now use m_ prefix to avoid shadowing warnings from GCC -Wshadow - other compile warnings eliminated as well (-Weffc++ etc) - documentation updates