{"action":"created","issue":{"url":"https://api.github.com/repos/robotology/yarp/issues/471","repository_url":"https://api.github.com/repos/robotology/yarp","labels_url":"https://api.github.com/repos/robotology/yarp/issues/471/labels{/name}","comments_url":"https://api.github.com/repos/robotology/yarp/issues/471/comments","events_url":"https://api.github.com/repos/robotology/yarp/issues/471/events","html_url":"https://github.com/robotology/yarp/pull/471","id":66168216,"number":471,"title":"[ServerSerial]: replaced Thread with RateThread; fix closing issue","user":{"login":"pattacini","id":3738070,"avatar_url":"https://avatars.githubusercontent.com/u/3738070?v=3","gravatar_id":"","url":"https://api.github.com/users/pattacini","html_url":"https://github.com/pattacini","followers_url":"https://api.github.com/users/pattacini/followers","following_url":"https://api.github.com/users/pattacini/following{/other_user}","gists_url":"https://api.github.com/users/pattacini/gists{/gist_id}","starred_url":"https://api.github.com/users/pattacini/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pattacini/subscriptions","organizations_url":"https://api.github.com/users/pattacini/orgs","repos_url":"https://api.github.com/users/pattacini/repos","events_url":"https://api.github.com/users/pattacini/events{/privacy}","received_events_url":"https://api.github.com/users/pattacini/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":{"login":"randaz81","id":2869969,"avatar_url":"https://avatars.githubusercontent.com/u/2869969?v=3","gravatar_id":"","url":"https://api.github.com/users/randaz81","html_url":"https://github.com/randaz81","followers_url":"https://api.github.com/users/randaz81/followers","following_url":"https://api.github.com/users/randaz81/following{/other_user}","gists_url":"https://api.github.com/users/randaz81/gists{/gist_id}","starred_url":"https://api.github.com/users/randaz81/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/randaz81/subscriptions","organizations_url":"https://api.github.com/users/randaz81/orgs","repos_url":"https://api.github.com/users/randaz81/repos","events_url":"https://api.github.com/users/randaz81/events{/privacy}","received_events_url":"https://api.github.com/users/randaz81/received_events","type":"User","site_admin":false},"milestone":null,"comments":21,"created_at":"2015-04-03T15:32:20Z","updated_at":"2016-05-11T09:29:30Z","closed_at":"2016-05-10T19:59:16Z","pull_request":{"url":"https://api.github.com/repos/robotology/yarp/pulls/471","html_url":"https://github.com/robotology/yarp/pull/471","diff_url":"https://github.com/robotology/yarp/pull/471.diff","patch_url":"https://github.com/robotology/yarp/pull/471.patch"},"body":"`ServerSerial` class now inherits from `RateThread` and no longer from `Thread`. This is possible since the class already was meant to be periodic with a period of `0.01 s`.\r\n\r\nThis way we call `askToStop` and we should be safer on closure.\r\n\r\n@alecive could you test it when you're available, since you've been so eager to open #469 :smirk: ?\n<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-89354757%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-89356061%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99580818%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99752977%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99755833%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99757251%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99767737%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99773576%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99829565%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99830148%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99860470%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99861650%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99876234%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-99876993%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-124820669%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-124820697%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-137126851%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-196240877%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-196241887%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-218272988%22%2C%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-218281575%22%5D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/robotology/yarp/pull/471%23issuecomment-89354757%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22This%20didn%27t%20fix%20%23469%20%2C%20but%20I%20would%20merge%20it%20anyway%20%3A%29%22%2C%20%22created_at%22%3A%20%222015-04-03T17%3A00%3A08Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/4378663%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/alecive%22%7D%7D%2C%20%7B%22body%22%3A%20%22Let%27s%20rather%20keep%20it%20for%20further%20experiments.%22%2C%20%22created_at%22%3A%20%222015-04-03T17%3A03%3A49Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3738070%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/pattacini%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40alecive%20%5Cr%5CnAdded%20ACE%20conditions%20%28now%20code%20is%20almost%20correct%20%3A%29%20%29%5Cr%5CnStill%20I%27m%20not%20convinced%20by%20this%20line%20%28which%20is%20not%20synchronized%2C%20but%20I%20hope%20this%20is%20not%20so%20problematic%29%5Cr%5Cnhttps%3A//github.com/robotology/yarp/blob/fix/server_serial/src/modules/serial/SerialDeviceDriver.cpp%23L229%5Cr%5Cn%5Cr%5CnMust%20be%20compiled%20and%20tested%20on%20the%20real%20robot%22%2C%20%22created_at%22%3A%20%222015-05-06T19%3A28%3A46Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2189180%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/francesco-romano%22%7D%7D%2C%20%7B%22body%22%3A%20%22The%20issue%20seems%20to%20be%20that%20the%20device%20thread%20is%20waiting%20on%20a%20socket%20%28%60recv%60%29%20in%20a%20blocking%20way.%5Cr%5CnThe%20close%20method%20is%20called%20from%20another%20thread%20and%20it%20happily%20removes%20all%20the%20objects%2C%20but%20does%20not%20unblock%20the%20thread.%5Cr%5CnFirst%20test%20I%20did%20was%20to%20send%20a%20%60SIGINT%60%20%20to%20the%20blocked%20thread.%20Of%20course%20a%20segfault%20happened.%5Cr%5CnSo%20I%27ve%20now%20synchronized%20the%20close%20of%20the%20driver%20with%20the%20return%20from%20the%20%60recv%60%20method.%5Cr%5CnUnfortunately%20to%20do%20this%20I%20needed%20to%20change%20the%20%28indefinitely%29%20blocking%20read%20with%20a%20read%20with%20timeout.%5Cr%5CnIf%20there%20are%20other%20ways%20to%20unlock%20the%20thread%20they%20can%20be%20easily%20substituted%20in%20the%20code%20instead%20of%20the%20timeout.%5Cr%5Cn%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222015-05-07T07%3A19%3A04Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2189180%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/francesco-romano%22%7D%7D%2C%20%7B%22body%22%3A%20%22Albeit%20sub-optimal%2C%20this%20is%20a%20good%20point%20to%20start%20from%3A%20at%20least%20you%20have%20found%20where%20we%20should%20look%20in%20order%20to%20fix%20the%20issue.%20Two%20more%20comments%3A%5Cr%5Cn%5Cr%5Cn%20%2A%20I%20tested%20%60yarpdev%20--name%20/icub/face/raw%20--device%20serial%20--subdevice%20serialport%20--context%20faceExpressions%20--from%20serialport.ini%60%20%28i.e.%20the%20module%20that%20handles%20face%20expressions%29%20and%20everything%20seems%20to%20work%20fine%20%28with%20the%20plus%20of%20being%20able%20to%20close%20the%20module%20itself%20%3A%2B1%3A%20%29%5Cr%5Cn%20%2A%20Since%20the%20serial%20is%20used%20not%20only%20for%20face%20expressions%2C%20we%20should%20test%20it%20in%20other%20parts%20of%20the%20SW%20that%20use%20it%2C%20but%20we%20don%27t%20know%20where%20to%20look%20for.%20%40randaz81%20%40lornat75%20%20do%20you%20have%20any%20hint%3F%22%2C%20%22created_at%22%3A%20%222015-05-07T07%3A34%3A40Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/4378663%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/alecive%22%7D%7D%2C%20%7B%22body%22%3A%20%22Great%20job%20guys%3A%20it%20was%20a%20nasty%20bug%20indeed.%5Cr%5Cn%5Cr%5CnThe%20drawback%20of%20the%20timeout%20is%20that%20it%27ll%20expose%20this%20component%20to%20run-time%20problems%20if%20we%20don%27t%20have%20an%20exact%20knowledge%20of%20the%20timing%20needed%20by%20the%20process.%20I%20imagine%20that%20the%20serial%20device%20can%20be%20programmed%20in%20many%20different%20configurations%2C%20which%20can%20thus%20require%20different%20timing.%20Is%20that%20right%3F%20Do%20we%20have%20other%20caveats%3F%5Cr%5Cn%5Cr%5CnCurrently%2C%20we%20have%20a%20deterministic%20failure%20only%20upon%20closure%2C%20which%20is%20less%20risky%20than%20having%20a%20non%20null%20probability%20of%20failure%20at%20run-time.%22%2C%20%22created_at%22%3A%20%222015-05-07T07%3A42%3A32Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3738070%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/pattacini%22%7D%7D%2C%20%7B%22body%22%3A%20%22We%20could%20try%20another%20test.%5Cr%5CnWe%20can%20print%20the%20return%20value%20of%20%60recv%60%20%28without%20timeout%29%20and%20of%20%60errno%60%20if%20the%20returned%20value%20is%20%60-1%60.%5Cr%5CnIf%20the%20error%20matches%20%60EINTR%60%2C%20then...%20at%20least%20we%20know%20that%20the%20interrupt%20signal%20is%20received.%20How%20to%20handle%20it%20is%20another%20story.%22%2C%20%22created_at%22%3A%20%222015-05-07T08%3A21%3A15Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2189180%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/francesco-romano%22%7D%7D%2C%20%7B%22body%22%3A%20%22This%20should%20be%20tested%20on%20all%20hw%20devices%20which%20use%20the%20serial%20port.%5Cr%5CnBesides%20the%20face%20expressions%2C%20I%20remember%20laser%20hokuyo%2C%20the%20ikart%20battery%20management%20system%2C%20the%20iCubGenova02%20battery%20management%20system.%20Not%20sure%20if%20there%20are%20others.%22%2C%20%22created_at%22%3A%20%222015-05-07T08%3A38%3A01Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2869969%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/randaz81%22%7D%7D%2C%20%7B%22body%22%3A%20%22If%20the%20read%20is%20done%20in%20a%20periodic%20thread%20maybe%20you%20can%20replace%20the%20blocking%20read%20with%20a%20simple%20polling%20that%20check%20and%20read%20only%20if%20data%20is%20available.%20%22%2C%20%22created_at%22%3A%20%222015-05-07T11%3A43%3A20Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1403333%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lornat75%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40lornat75%20raised%20a%20good%20point%3A%20if%20we%20use%20a%20%60yarp%3A%3Aos%3A%3ARateThread%60%20%28that%20was%20basically%20the%20case%20also%20before%20with%20a%20normal%20%60yarp%3A%3Aos%3A%3AThread%60%20alongside%20with%20a%20%60Time%3A%3Adelay%28%29%60%29%2C%20then%20why%20do%20we%20have%20a%20blocking%20%60read%28%29%60%3F%22%2C%20%22created_at%22%3A%20%222015-05-07T11%3A48%3A20Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3738070%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/pattacini%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40lornat75%20%40pattacini%20%5Cr%5CnMaybe%20I%20did%20not%20understand%20correctly%20your%20point%20%28also%20because%20the%20amount%20of%20Driver/PolyDriver%20opened%20by%20this%20module%20makes%20things%20little%20confusing%29.%5Cr%5Cn%5Cr%5CnAt%20run%20time%20there%20is%20the%20%60ServerSerial%60%2C%20which%20is%20now%20a%20%60RateThread%60%20which%20at%20each%20%60run%28%29%60%20calls%20the%20%60receive%60%20method%20on%20the%20SerialDeviceDriver%2C%20which%20in%20turn%20calls%20a%20blocking%20%60recv%60%20on%20the%20socket.%5Cr%5CnI%20simply%20tried%20to%20emulate%20the%20same%20behaviour%20but%20with%20a%20timeout%20on%20the%20blocking%20%60recv%60%20just%20to%20be%20able%20to%20properly%20close%20the%20module.%20%22%2C%20%22created_at%22%3A%20%222015-05-07T13%3A16%3A09Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2189180%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/francesco-romano%22%7D%7D%2C%20%7B%22body%22%3A%20%22we%27ve%20clarified%20_de%20visu_.%22%2C%20%22created_at%22%3A%20%222015-05-07T13%3A20%3A56Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3738070%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/pattacini%22%7D%7D%2C%20%7B%22body%22%3A%20%22Is%20there%20a%20parameter%20to%20specify%20the%20period%3F%5Cr%5CnWe%20are%20trying%20to%20have%20all%20the%20wrapper/server%20similar%20parameters%2C%20for%20period%20usually%20we%20have%20the%20keyword%20period%20in%20units%20of%20ms%20%20%28except%20for%20serverInertial%20in%20which%20the%20unit%20is%20seconds%29.%5Cr%5Cn%5Cr%5CnIt%20would%20be%20good%20if%20this%20device%20complies%20with%20the%20%27period%20ms%27%20way%5Cr%5Cn%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222015-05-07T13%3A54%3A57Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/4061020%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/barbalberto%22%7D%7D%2C%20%7B%22body%22%3A%20%22Nope%2C%20there%20wasn%27t%20any%20%60period%60%20option%2C%20though%20I%20think%20it%20could%20be%20added%20easily.%22%2C%20%22created_at%22%3A%20%222015-05-07T13%3A56%3A35Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3738070%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/pattacini%22%7D%7D%2C%20%7B%22body%22%3A%20%22Can%20we%20merge%20this%3F%22%2C%20%22created_at%22%3A%20%222015-07-25T08%3A31%3A19Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1403333%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lornat75%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20think%20%40randaz81%20wanted%20to%20test%20it%20with%20other%20devices%20before%20merging...%22%2C%20%22created_at%22%3A%20%222015-07-25T08%3A32%3A11Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3738070%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/pattacini%22%7D%7D%2C%20%7B%22body%22%3A%20%22Result%20of%20discussion%20with%20%40randaz81%20%5Cr%5Cn%5Cr%5Cn%60SerialDeviceDriver%60%20is%20not%20used%20only%20by%20%60ServerSerial%60%2C%20but%20it%20is%20also%20used%20directly%20%28e.g.%20laser%29.%5Cr%5CnChanging%20the%20receive%20from%20blocking%20to%20non-blocking%20will%20completely%20change%20the%20expected%20behaviour.%5Cr%5CnWe%20should%20then%20revert%20to%20the%20old%20implementation%3A%20blocking%20with%20%28hidden%29%20timeout%2C%20plus%20synchronization%20variables%20to%20perform%20a%20clean%20close%20of%20the%20device.%5Cr%5Cn%5Cr%5CnWe%20can%20also%20add%20a%20new%20%60receiveWithTimeout%60%20method%20%28also%20in%20the%20%60ISerialDevice%60%20interface%29%20and%20let%20the%20%60ServerSerial%60%20call%20this%20method%5Cr%5Cn%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222015-09-02T15%3A20%3A38Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2189180%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/francesco-romano%22%7D%7D%2C%20%7B%22body%22%3A%20%22This%20pull%20request%20has%20been%20open%20for%20almost%20a%20year%20and%20was%20never%20merged.%20Should%20it%20be%20closed%20or%20is%20it%20waiting%20to%20be%20tested%3F%5Cr%5CnAnyway%2C%20at%20the%20moment%20it%20has%20conflicts%20and%20needs%20to%20be%20rebased%20over%20the%20latest%20master.%22%2C%20%22created_at%22%3A%20%222016-03-14T10%3A16%3A47Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1100056%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/drdanz%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20talked%20with%20%40randaz81%20some%20weeks%20ago%2C%20and%20he%20thinks%20that%20probably%20it%20is%20better%20to%20completely%20change%20the%20face%20expressions%20module%2C%20instead%20of%20changing%20the%20ServerSerial%20driver.%5Cr%5CnThis%20is%20why%20this%20PR%20is%20in%20standby.%20%22%2C%20%22created_at%22%3A%20%222016-03-14T10%3A20%3A38Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2189180%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/francesco-romano%22%7D%7D%2C%20%7B%22body%22%3A%20%22Blind%20alley.%22%2C%20%22created_at%22%3A%20%222016-05-10T19%3A59%3A16Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3738070%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/pattacini%22%7D%7D%2C%20%7B%22body%22%3A%20%22%5Cud83d%5Cude22%20%22%2C%20%22created_at%22%3A%20%222016-05-10T20%3A30%3A37Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/2189180%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/francesco-romano%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%7D%7D'></a>\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\n- [ ] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/robotology/yarp/pull/471#issuecomment-89354757'>General Comment</a></b>\n- <a href='https://github.com/alecive'><img border=0 src='https://avatars.githubusercontent.com/u/4378663?v=3' height=16 width=16'></a> This didn't fix #469 , but I would merge it anyway :)\n- <a href='https://github.com/pattacini'><img border=0 src='https://avatars.githubusercontent.com/u/3738070?v=3' height=16 width=16'></a> Let's rather keep it for further experiments.\n- <a href='https://github.com/francesco-romano'><img border=0 src='https://avatars.githubusercontent.com/u/2189180?v=3' height=16 width=16'></a> @alecive\nAdded ACE conditions (now code is almost correct :) )\nStill I'm not convinced by this line (which is not synchronized, but I hope this is not so problematic)\nhttps://github.com/robotology/yarp/blob/fix/server_serial/src/modules/serial/SerialDeviceDriver.cpp#L229\nMust be compiled and tested on the real robot\n- <a href='https://github.com/francesco-romano'><img border=0 src='https://avatars.githubusercontent.com/u/2189180?v=3' height=16 width=16'></a> The issue seems to be that the device thread is waiting on a socket (`recv`) in a blocking way.\nThe close method is called from another thread and it happily removes all the objects, but does not unblock the thread.\nFirst test I did was to send a `SIGINT`  to the blocked thread. Of course a segfault happened.\nSo I've now synchronized the close of the driver with the return from the `recv` method.\nUnfortunately to do this I needed to change the (indefinitely) blocking read with a read with timeout.\nIf there are other ways to unlock the thread they can be easily substituted in the code instead of the timeout.\n- <a href='https://github.com/alecive'><img border=0 src='https://avatars.githubusercontent.com/u/4378663?v=3' height=16 width=16'></a> Albeit sub-optimal, this is a good point to start from: at least you have found where we should look in order to fix the issue. Two more comments:\n* I tested `yarpdev --name /icub/face/raw --device serial --subdevice serialport --context faceExpressions --from serialport.ini` (i.e. the module that handles face expressions) and everything seems to work fine (with the plus of being able to close the module itself :+1: )\n* Since the serial is used not only for face expressions, we should test it in other parts of the SW that use it, but we don't know where to look for. @randaz81 @lornat75  do you have any hint?\n- <a href='https://github.com/pattacini'><img border=0 src='https://avatars.githubusercontent.com/u/3738070?v=3' height=16 width=16'></a> Great job guys: it was a nasty bug indeed.\nThe drawback of the timeout is that it'll expose this component to run-time problems if we don't have an exact knowledge of the timing needed by the process. I imagine that the serial device can be programmed in many different configurations, which can thus require different timing. Is that right? Do we have other caveats?\nCurrently, we have a deterministic failure only upon closure, which is less risky than having a non null probability of failure at run-time.\n- <a href='https://github.com/francesco-romano'><img border=0 src='https://avatars.githubusercontent.com/u/2189180?v=3' height=16 width=16'></a> We could try another test.\nWe can print the return value of `recv` (without timeout) and of `errno` if the returned value is `-1`.\nIf the error matches `EINTR`, then... at least we know that the interrupt signal is received. How to handle it is another story.\n- <a href='https://github.com/randaz81'><img border=0 src='https://avatars.githubusercontent.com/u/2869969?v=3' height=16 width=16'></a> This should be tested on all hw devices which use the serial port.\nBesides the face expressions, I remember laser hokuyo, the ikart battery management system, the iCubGenova02 battery management system. Not sure if there are others.\n- <a href='https://github.com/lornat75'><img border=0 src='https://avatars.githubusercontent.com/u/1403333?v=3' height=16 width=16'></a> If the read is done in a periodic thread maybe you can replace the blocking read with a simple polling that check and read only if data is available.\n- <a href='https://github.com/pattacini'><img border=0 src='https://avatars.githubusercontent.com/u/3738070?v=3' height=16 width=16'></a> @lornat75 raised a good point: if we use a `yarp::os::RateThread` (that was basically the case also before with a normal `yarp::os::Thread` alongside with a `Time::delay()`), then why do we have a blocking `read()`?\n- <a href='https://github.com/francesco-romano'><img border=0 src='https://avatars.githubusercontent.com/u/2189180?v=3' height=16 width=16'></a> @lornat75 @pattacini\nMaybe I did not understand correctly your point (also because the amount of Driver/PolyDriver opened by this module makes things little confusing).\nAt run time there is the `ServerSerial`, which is now a `RateThread` which at each `run()` calls the `receive` method on the SerialDeviceDriver, which in turn calls a blocking `recv` on the socket.\nI simply tried to emulate the same behaviour but with a timeout on the blocking `recv` just to be able to properly close the module.\n- <a href='https://github.com/pattacini'><img border=0 src='https://avatars.githubusercontent.com/u/3738070?v=3' height=16 width=16'></a> we've clarified _de visu_.\n- <a href='https://github.com/barbalberto'><img border=0 src='https://avatars.githubusercontent.com/u/4061020?v=3' height=16 width=16'></a> Is there a parameter to specify the period?\nWe are trying to have all the wrapper/server similar parameters, for period usually we have the keyword period in units of ms  (except for serverInertial in which the unit is seconds).\nIt would be good if this device complies with the 'period ms' way\n- <a href='https://github.com/pattacini'><img border=0 src='https://avatars.githubusercontent.com/u/3738070?v=3' height=16 width=16'></a> Nope, there wasn't any `period` option, though I think it could be added easily.\n- <a href='https://github.com/lornat75'><img border=0 src='https://avatars.githubusercontent.com/u/1403333?v=3' height=16 width=16'></a> Can we merge this?\n- <a href='https://github.com/pattacini'><img border=0 src='https://avatars.githubusercontent.com/u/3738070?v=3' height=16 width=16'></a> I think @randaz81 wanted to test it with other devices before merging...\n- <a href='https://github.com/francesco-romano'><img border=0 src='https://avatars.githubusercontent.com/u/2189180?v=3' height=16 width=16'></a> Result of discussion with @randaz81\n`SerialDeviceDriver` is not used only by `ServerSerial`, but it is also used directly (e.g. laser).\nChanging the receive from blocking to non-blocking will completely change the expected behaviour.\nWe should then revert to the old implementation: blocking with (hidden) timeout, plus synchronization variables to perform a clean close of the device.\nWe can also add a new `receiveWithTimeout` method (also in the `ISerialDevice` interface) and let the `ServerSerial` call this method\n- <a href='https://github.com/drdanz'><img border=0 src='https://avatars.githubusercontent.com/u/1100056?v=3' height=16 width=16'></a> This pull request has been open for almost a year and was never merged. Should it be closed or is it waiting to be tested?\nAnyway, at the moment it has conflicts and needs to be rebased over the latest master.\n- <a href='https://github.com/francesco-romano'><img border=0 src='https://avatars.githubusercontent.com/u/2189180?v=3' height=16 width=16'></a> I talked with @randaz81 some weeks ago, and he thinks that probably it is better to completely change the face expressions module, instead of changing the ServerSerial driver.\nThis is why this PR is in standby.\n- <a href='https://github.com/pattacini'><img border=0 src='https://avatars.githubusercontent.com/u/3738070?v=3' height=16 width=16'></a> Blind alley.\n- <a href='https://github.com/francesco-romano'><img border=0 src='https://avatars.githubusercontent.com/u/2189180?v=3' height=16 width=16'></a> 😢\n\n\n<a href='https://www.codereviewhub.com/robotology/yarp/pull/471?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/robotology/yarp/pull/471?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/robotology/yarp/pull/471'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\n<a href='#crh-end'></a>"},"comment":{"url":"https://api.github.com/repos/robotology/yarp/issues/comments/218408371","html_url":"https://github.com/robotology/yarp/pull/471#issuecomment-218408371","issue_url":"https://api.github.com/repos/robotology/yarp/issues/471","id":218408371,"user":{"login":"randaz81","id":2869969,"avatar_url":"https://avatars.githubusercontent.com/u/2869969?v=3","gravatar_id":"","url":"https://api.github.com/users/randaz81","html_url":"https://github.com/randaz81","followers_url":"https://api.github.com/users/randaz81/followers","following_url":"https://api.github.com/users/randaz81/following{/other_user}","gists_url":"https://api.github.com/users/randaz81/gists{/gist_id}","starred_url":"https://api.github.com/users/randaz81/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/randaz81/subscriptions","organizations_url":"https://api.github.com/users/randaz81/orgs","repos_url":"https://api.github.com/users/randaz81/repos","events_url":"https://api.github.com/users/randaz81/events{/privacy}","received_events_url":"https://api.github.com/users/randaz81/received_events","type":"User","site_admin":false},"created_at":"2016-05-11T09:29:30Z","updated_at":"2016-05-11T09:29:30Z","body":"I'm restoring the branch to remember that this issue is still to be fixed, however some changes have to be done to these commits."}}