From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <53C44696.2070906@xenomai.org> Date: Mon, 14 Jul 2014 17:07:34 -0400 From: Jorge Ramirez Ortiz MIME-Version: 1.0 References: <53C3E6CE.2040306@xenomai.org> <53C42ADE.7040204@gmail.com> In-Reply-To: <53C42ADE.7040204@gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai] analogy: sensory 626 patch [Re: Xenomai Digest, Vol 27, Issue 12] List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wojciech Domski , xenomai@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 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: >>>>> >>>> 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