Re: [Xenomai] analogy: sensory 626 patch [Re: Xenomai Digest, Vol 27, Issue 12]

[Jorge Ramirez Ortiz <jro@xenomai.org>]

On 07/14/2014 03:09 PM, Wojciech Domski wrote:
> W dniu 2014-07-14 16:18, Jorge Ramirez Ortiz pisze:
>> ---------------------------------------------------------------------- Message: 1 
>> Date: Mon, 07 Jul 2014 12:38:42 +0200 From: Wojciech Domski <> To: Gilles 
>> Chanteperdrix <gilles.chanteperdrix@xenomai.org> Cc: xenomai@xenomai.org Subject: 
>> Re: [Xenomai] Sensoray 626 analogy driver Message-ID: <53BA78B2.2010802@gmail.com> 
>> Content-Type: text/plain; charset="utf-8"; Format="flowed" W dniu 11.04.2014 
>> 00:01, Gilles Chanteperdrix pisze:
>>>>
>>>> On 04/10/2014 02:25 PM, Wojciech Domski wrote:
>>>>> 2014-03-25 13:33 GMT+01:00 Gilles Chanteperdrix <
>>>>> gilles.chanteperdrix@xenomai.org>:
>>>>>
>>>>>>
>>>>>> On 03/25/2014 01:14 PM, Wojciech Domski wrote:
>>>>>>> W dniu 25.03.2014 12:58, Gilles Chanteperdrix pisze:
>>>>>>>>
>>>>>>>> On 03/25/2014 12:54 PM, Wojciech Domski wrote:
>>>>>>>>> W dniu 25.03.2014 12:36, Gilles Chanteperdrix pisze:
>>>>>>>>>>
>>>>>>>>>> On 03/25/2014 12:28 PM, Wojciech Domski wrote:
>>>>>>>>>>> W dniu 25.03.2014 12:16, Gilles Chanteperdrix pisze:
>>>>>>>>>>>>
>>>>>>>>>>>> On 03/25/2014 11:26 AM, Wojciech Domski wrote:
>>>>>>>>>>>>> W dniu 24.03.2014 13:19, Gilles Chanteperdrix pisze:
>>>>>>>>>>>>>> The solution which would be acceptable is not to have busy waits,
>>>>>> except
>>>>>>>>>>>>>> for very short durations. But for instance transferring a byte on
>>>>>> I2C
>>>>>>>>>>>>>> takes around 20us at 400 kHz, a 20us masking section is
>>>>>> unacceptable.
>>>>>>>>>>>>>> rtdm_task_busy_sleep, as the name suggests is a busy wait loop,
>>>>>> so, no,
>>>>>>>>>>>>>> it is not acceptable either.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Use a thread or a timer to reschedule while you wait for the end
>>>>>> of the
>>>>>>>>>>>>>> transfer, instead of busy waiting.
>>>>>>>>>>>>> Dear Gilles,
>>>>>>>>>>>>>
>>>>>>>>>>>>> As you mentioned before the driver has few places like this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> while(!FLAG1);
>>>>>>>>>>>>> while(!FLAG2);
>>>>>>>>>>>>>
>>>>>>>>>>>>> Would a solution of creating a separate task for this purpose be
>>>>>> ok?
>>>>>>>>>>>>> task()
>>>>>>>>>>>>> {
>>>>>>>>>>>>>           while(!FLAG1);
>>>>>>>>>>>>>           while(!FLAG2);
>>>>>>>>>>>>> }
>>>>>>>>>>>> I was rather thinking about rtdm_task_sleep() instead.
>>>>>>>>>>>>
>>>>>>>>>>>>> In the place of those loops a piece of code responsible for
>>>>>> creating and
>>>>>>>>>>>>> joining task would be put instead:
>>>>>>>>>>>>>
>>>>>>>>>>>>> rtdm_task_init();
>>>>>>>>>>>>> rtdm_task_join_nrt();
>>>>>>>>>>>> This will not work for code currently running in interrupt
>>>>>> handlers. An
>>>>>>>>>>>> interrupt handler can not call rtdm_task_join_nrt(). You will rather
>>>>>>>>>>>> have to wake up the thread, not reenabling the interrupt at the end
>>>>>> of
>>>>>>>>>>>> the handler, and reenable the interrupt in the thread when the job
>>>>>> is
>>>>>>>>>>>> done. Or alternatively, fire a timer instead of waking up a thread.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>> Dear Gilles,
>>>>>>>>>>>
>>>>>>>>>>> If what I have understood correctly, basically what you mean is
>>>>>> changing:
>>>>>>>>>>> while(!FLAG) ;
>>>>>>>>>>>
>>>>>>>>>>> into
>>>>>>>>>>>
>>>>>>>>>>> while(!FLAG)
>>>>>>>>>>>          rtdm_task_sleep();
>>>>>>>>>>>
>>>>>>>>>>> Xenomai's documentation that rtdm_task_sleep() can be always
>>>>>>>>>>> rescheduled. In my opinion
>>>>>>>>>>> this solution is sufficient and meets Xenomai requirements.
>>>>>>>>>> This solution will not work if the busy loop was in an irq handler.
>>>>>> You
>>>>>>>>>> can not call rtdm_task_sleep from the context of an irq handler.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Ok, now I get your point.
>>>>>>>>>
>>>>>>>>> However, what do you mean exactly by rescheduling while waiting? Do you
>>>>>>>>> mean something like
>>>>>>>>> putting all irq services into another thread and only in irq handler
>>>>>>>>> notify the separate thread
>>>>>>>>> to do the job accordingly?
>>>>>>>> Well, I do not know exactly, I have not looked at the details of the
>>>>>>>> driver, you are supposed to do that. But you may be able to split the
>>>>>>>> interrupt code in two parts: a first part that will run in interrupt
>>>>>>>> mode, and a second part that will run as a thread. The irq handler would
>>>>>>>> wake the thread up at the end of the first part, and return without
>>>>>>>> reenabling the interrupt at interrupt controller level. The thread would
>>>>>>>> execute, sleep during the I2C transfers as needed, and reenable the
>>>>>>>> interrupt before going back to sleep.
>>>>>>>>
>>>>>>>>
>>>>>>> So would it be a good practise to create a new task for irq service with
>>>>>>>
>>>>>>> rtdm_task_init();
>>>>>>>
>>>>>>> inside attach function and join it with
>>>>>>>
>>>>>>> rtdm_task_join_nrt();
>>>>>>>
>>>>>>> inside detach function?
>>>>>> I do not know the details about analogy drivers, so, again, it is left
>>>>>> to you to read other analogy drivers and see where is the best place to
>>>>>> do what you want to do.
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Gilles.
>>>>>>
>>>>> Dear Gilles,
>>>>>
>>>>> I have altered the driver according to your advices.
>>>>> If I had omitted something please let me know.
>>>>>
>>>>> Please find a patch enclosed in the attachment.
>>>>>
>>>>> Best regards,
>>>>> Wojciech Domski
>>>>>
>>>>> Domski.pl
>>>>>
>>>>> Wojciech.Domski.pl
>>>>> -------------- next part --------------
>>>>> A non-text attachment was scrubbed...
>>>>> Name: s626_patch.patch
>>>>> Type: text/x-patch
>>>>> Size: 141659 bytes
>>>>> Desc: not available
>>>>> URL: 
>>>>> <http://www.xenomai.org/pipermail/xenomai/attachments/20140410/e7191247/attachment.bin>
>>>> scripts/checkpatch.pl attchment.bin:
>>>>
>>>> total: 325 errors, 392 warnings, 3991 lines checked
>>>>
>>>> Please fix these errors.
>>>>
>>>>
>>> Dear Gilles,
>>>
>>> In the attachment you will find patch for xenomai including support for
>>> analogy Sensoray 626.
>>>
>>> May I ask you to include this patch to main xenomai tree?
>>>
>>> Best regards,
>>>
>>> Wojciech Domski
>>>
>>> Domski.pl
>>>
>>> Wojciech.Domski.pl
>>>
>>
>>
>> Hi Wojciech
>>
>> I did some changes to the patch that you sent and pushed it to my Xenomai 3 tree 
>> (branch sensoray) as work in progress; however we have no means to test or 
>> validate any of the changes.
>> http://git.xenomai.org/xenomai-jro.git/log/?h=sensoray
>>
>> is this something that you can do? ie, do you have access to the sensoray device?
>>
>> In any case after reviewing the Comedi implementation in 3.12 staging I think it 
>> would make sense to base off that newer release instead.
>>
>> jro
>>
>>
>>
> Thank you for committing the driver.
> I have access to the Sensoray 626 board and I will test the remarks you have 
> proposed. It will take me sometime, however. First of all I would like to test my 
> driver with two Sensoray boards. For now, I see that it won't work with more than 
> one card. It requires some changes to the attach and probe function. After I 
> implement this functionality I will take a look into the interrupt handling service.
>
> Seizing the opportunity, I would like to ask you a question about handling PCI 
> device. When I'm calling pci_register_driver() and I have two exactly the same PCI 
> devices will this function call appropriate probe() function two times? If yes, 
> then is the probe() function an appropriate place to make list of available devices?
>
> Wojciech Domski
>
> Domski.pl
>
> Wojciech.Domski.pl
>
>
invoking pci_register_driver in dev_s626_attach causes the probe interface in the pci 
driver to be executed for all the devices of the kind described in the pci id table; 
this is probably the reason why you can only register one.

It might be easier to follow the approach in analogy/national_instruments/mite.c

mite_init registers the driver which ends up calling mite_probe which will create a 
mite_struct and add it to the list of mite_devices.
Incidentally if you were wondering, this is the reason why if the pci_register_driver 
call succeeded the driver can display all the devices in the system.

the same would need to be done for the s626

on a different note, we are adding a new interface to rtdm to support the busy loops.
rtdm_busywait_safe(__condition, __busy_delay, __busy_wait)

this should allow you to do things like this

-       while (!MC_TEST(P_MC2, MC2_UPLD_DEBI))
-               rtdm_task_sleep(2000);
+       ret = rtdm_busywait_safe(!MC_TEST(P_MC2, MC2_UPLD_DEBI), BUSY_DELAY, BUSY_SLEEP);
+       if (ret)
+               __a4l_err( "error waiting");






-- 
jro