[alsa-devel] [PATCH 1/2] New Control Plugin - arcam-av

[Peter Stokes]

Thank you for taking the time to review my code.

On 4 Jan 2009, at 14:16, Takashi Iwai wrote:

> At Sun, 4 Jan 2009 12:37:11 +0000,
> Peter Stokes wrote:
>>
>> --- alsa-plugins-1.0.18-orig/arcam-av/arcam_av.c	1970-01-01  
>> 01:00:00.000000000
>> +0100
>> +++ alsa-plugins-1.0.18/arcam-av/arcam_av.c	2009-01-02  
>> 22:30:55.000000000
>> +0000
>
> Looks like your embedded patches are broken due to line-breaks by MUA.
> Please fix it, or use attachments if it's difficult.
>

I'll try attaching it. Please find attached an updated patch.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arcam-av.patch
Type: application/octet-stream
Size: 50595 bytes
Desc: not available
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20090104/9601097f/attachment-0001.dll 
-------------- next part --------------



>> +int arcam_av_connect(const char* port)
>> +{
>> +	int fd = open(port, O_RDWR | O_NOCTTY);
>> +	if (fd < 0)
>> +		return -errno;
>> +
>> +	struct termios portsettings;
>
> Put this into the beginning to follow the standard C.
>

Sorry, I am more used to programming in C++. I have attempted to move  
all of the variable declarations to the top of scope sections, but I  
may have missed some. I assume that was what you meant?

>
>> +	if (bytes == 7)
>> +		return 0;
>> +	else if (bytes >= 0)
>> +		return -1;
>
> No proper error code?
>

I have modified this function to hopefully make it more robust.

>> +static int arcam_av_receive(int fd, arcam_av_cc_t* command,  
>> unsigned char*
>> param1, unsigned char* param2)
>> +{
>> +	static int index = 0;
>> +	static arcam_av_cc_t received_command;
>> +	static unsigned char received_param1;
>> +	static unsigned char received_param2;
>
> Are static variables safe to use here?
> That is, is the plugin designed to access exclusively with a proper
> protection?
>
>>

I believe so, yes.

The system is intended to broadly function as follows:

Each invocation of the plugin calls "arcam_av_connect()" to obtain a  
file descriptor for the serial device to be used to communicate with  
the amplifier. This file descriptor is used to send commands (requests  
to change state) directly to the amplifier (using the  
"arcam_av_send()" function). i.e. all invocations send directly to the  
amplifier.

Each invocation creates a server thread via the  
"arcam_av_server_start()" function. The server thread then switches  
into one of two state, master or slave. The server thread attempts to  
bind to a unix socket, if successful then there is currently no master  
server bound to the socket and the thread should become the master, if  
unsuccessful there is already a master bound to the socket and the  
thread should become a slave. The master server listens to the serial  
device for command responses from the amplifier (obtained using the  
"arcam_av_receive()" function). i.e. only the single master server  
receives directly from the amplifier.

Clients may connect to the master server via the unix socket (using  
the "arcam_av_client()" function). When the master server receives a  
successful command response from the amplifier it notifies all current  
clients that a change has occurred. This mechanism allows many clients  
(invocations of the plugin) to be notified of the change.

Slave servers also attach to the master server as a client. This  
connection is simply used to detect whether the master server has died  
as a result of the plugin invocation housing it being terminated. If  
the master server dies any slave servers attempt to bind to the unix  
socket and in doing so one would become the master server, any  
additional slaves would remain slaves.


>
> Well, I'll review the rest after my vacation is over...
>
>

Thanks! Enjoy your vacation.

Peter