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 errorstring 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 thiscase, 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 betweenplayback and capture is so big that we cannot make one of them thedefault.
> (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 devicename, 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 tocheck the error status of the stream (because most errors aren'treported by exceptions). That this also allows chaining is just a sideeffect.
> > > 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 wantsto read the current setting or to begin choosing new settings. It isnot 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 initialor current settings are loaded from a PCM device. But then thehw_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 eithercommitted or rolled back. The usual idiom seems to be a RAII classwhose destructor rolls back if a commit or rollback hasn't alreadyhappended.
In our case, a rollback is equivalent to calling snd_pcm_mmap_commitwith frames==0.
Hmm, it should be possible to commit any number of frames as long as itisn't more than originally requested.
> 2. 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 theALSA API, and offering an easier-to-use API that is not as low-levelas the ALSA API itself. I think these two should be put in separateheader files.
Anyway, we need more wrappers for all those data structures. I'll seewhat I can come up with.
Regards,Clemens_______________________________________________Alsa-devel mailing listAlsa-devel@xxxxxxxxxxxxxxxxxxxx://mailman.alsa-project.org/mailman/listinfo/alsa-devel
