Lasse Kärkkäinen wrote:
Thank you for your input, which is quite valuable.
error(std::string const& function, int err): std::runtime_error("ALSA " + function + " failed: " + std::string(snd_strerror(err))), err(err) {}
I'm not sure that including the function name and all parameters as they appear in the source file in the error message is a good idea; I wouldn't want to impose this policy on all applications using this header.
It is just an exception thrown, the program may catch it and display another error message. The original ALSA error code is also included within it.
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.
In practice the function name seems to be quite essential for debugging, as EPIPE or some other of the standard error codes will tell little or nothing.
Agreed.
For completeness, I think that it might be desirable to also store the function name separately (like the code is stored), but can anyone see any actual use for this?
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.
pcm(char const* device = "default", snd_pcm_stream_t stream = SND_PCM_STREAM_PLAYBACK, int mode = 0) {
This looks too much like a default constructor. I think neither device nor stream should have a default value.
Well, it is a default constructor. One design goal was to make this wrapper as easy to use as possible, and choosing sensible defaults for that is helpful.
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.
(where "default" and playback are the most common choices; and also far better than people hardcoding their applications to use "plughw:0" or whatever happens to work for them).
In practice, most applications allow the user to specify the device name, so the default value won't be used anyway.
If you are expecting alsa::pcm pcmobject; to produce an object that is not constructed, ...
No, I'm not.
ALSA_HPP_CLASS& set_##name(...) { ...; return *this; }\
Is there a reason why you return the object itself? Usually, returning an object implies that that object is a _different_ object with the setting applied, which would mean that the original object was _not_ changed, but that isn't true here.
This is for function call chaining. It is a common idiom used by the standard library and others.
The STL streams use this to allow using the convert-to-bool operator to check the error status of the stream (because most errors aren't reported by exceptions). That this also allows chaining is just a side effect.
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.
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 ...
~mmap() { // We just assume that this works (can't do anything sensible if it fails). snd_pcm_mmap_commit(pcm, offset, frames);
This is the place where all the usual write errors must be checked. This cannot be done in a destructor. I think using RAII for the non- error commit just isn't possible.
Ouch. This is a big problem.
This is very similar to database transaction that must be either committed or rolled back. The usual idiom seems to be a RAII class whose destructor rolls back if a commit or rollback hasn't already happended.
In our case, a rollback is equivalent to calling snd_pcm_mmap_commit with frames==0.
Hmm, it should be possible to commit any number of frames as long as it isn't more than originally requested.
- Add a separate commit function
... Option 2 is hairy, as it leaves the object in unusable state after commit is called,
After committing, it is indeed not allowed to access the mmap buffer, so this isn't too unexpected.
In practice, the commit will be the last action before the destructor.
BTW: The object should be called mmap_access or something like that.
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.
Anyway, we need more wrappers for all those data structures. I'll see what I can come up with.
Regards, Clemens