{"action":"created","issue":{"url":"https://api.github.com/repos/Pi4J/pi4j/issues/140","labels_url":"https://api.github.com/repos/Pi4J/pi4j/issues/140/labels{/name}","comments_url":"https://api.github.com/repos/Pi4J/pi4j/issues/140/comments","events_url":"https://api.github.com/repos/Pi4J/pi4j/issues/140/events","html_url":"https://github.com/Pi4J/pi4j/pull/140","id":62220553,"number":140,"title":"New concurrency in I2CDeviceImpl and I2CBusImpl","user":{"login":"RasPelikan","id":10763265,"avatar_url":"https://avatars.githubusercontent.com/u/10763265?v=3","gravatar_id":"","url":"https://api.github.com/users/RasPelikan","html_url":"https://github.com/RasPelikan","followers_url":"https://api.github.com/users/RasPelikan/followers","following_url":"https://api.github.com/users/RasPelikan/following{/other_user}","gists_url":"https://api.github.com/users/RasPelikan/gists{/gist_id}","starred_url":"https://api.github.com/users/RasPelikan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RasPelikan/subscriptions","organizations_url":"https://api.github.com/users/RasPelikan/orgs","repos_url":"https://api.github.com/users/RasPelikan/repos","events_url":"https://api.github.com/users/RasPelikan/events{/privacy}","received_events_url":"https://api.github.com/users/RasPelikan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-03-16T21:59:13Z","updated_at":"2015-03-17T06:26:47Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/Pi4J/pi4j/pulls/140","html_url":"https://github.com/Pi4J/pi4j/pull/140","diff_url":"https://github.com/Pi4J/pi4j/pull/140.diff","patch_url":"https://github.com/Pi4J/pi4j/pull/140.patch"},"body":"Hello Robert,\r\n\r\nas I wrote in the Pi4J Google group (https://groups.google.com/forum/#!topic/pi4j/aWzcyzrFEHs) I want to refactor I2CDeviceImpl to allow write/reading to multiple busses in parallel without blocking each other. Now I’m ready and sum up what’s new:\r\n\r\nI2CFactory/I2CFactoryProvider/I2CFactoryProviderRaspberryPi/I2CFactoryProviderBananaPi:\r\n\r\nThe getInstance-methods throw an UnsupportedBusNumberException if the underlying system does not know the required bus-number. This is the only situation (of my changes) which effected customer’s code. Maybe you wish to remove this. At the moment, if a system knows about a bus but it isn’t configured it throws an IOException. This might be sufficient for unsupported bus-numbers, too. It’s up to you :-)\r\n\r\nI added a method getInstance(int, long, TimeUnit) which takes a locking-timeout for all actions on the bus required. Yes this is new: locking is not up the device it is up to the bus!\r\n\r\nI2CBusImplBananaPi/I2CBusImplRaspberryPi:\r\n\r\nI changed class hierarchy so I2CBusImpl now is an abstract class and requires a specific class for using it. I2CBusImplBananaPi and I2CBusImplRaspberryPi are those classes.\r\n\r\nI2CBus/I2CBusImpl:\r\n\r\nAs mentioned above now I2CBusImpl is an abstract class. I moved all read- and write-methods into this class but they are not part of I2CBus. They are part of the contract between I2CBusImpl and I2CDeviceImpl - maybe it is better to change the visibility to 'package' (now they are 'public'). I added a method ‚runActionOnExclusivLockedBus‘‚ which is used by I2CDeviceImpl for proper locking. This method may also be used by custom code to do several actions exclusively.\r\n\r\nWriting and reading to com.pi4j.jni.I2C is now up to I2CBusImpl. I think it applies better to the real-world because a software writes to or reads from the bus to communicate with a device. Additionally now every shell (I2CDeviceImpl and I2CBusImpl beneath) enriches the users request for reading or writing: I2CDeviceImpl adds the address and error-handling; I2CBusImpl adds the filedesciptor and locking.\r\n\r\nAdditionally I added some code for proper handling of closing the bus.\r\n\r\nI2CDeviceImpl:\r\n\r\nChanged internals to match changes of I2CBusImpl. There are some inner-classes implementing the I2CRunnable required by I2CBus#runActionOnExclusivLockedBus. Those classes also may have been anonymous classes but I decided to name them for better readability.\r\n\r\nTests:\r\n\r\nI added tests for I2CBusImpl and I2CDeviceImpl. For mocking of I2C I had to add powermock as a dependency and changed pom.xml to add a new JVM-parameter for unit-tests. Unfortunately there is a linker-problem reported on running the tests. This has to be ignored but it cannot be suppressed :-( -> I added a message which should calm the user :-)\r\n\r\nI run the tests on a dual-core computer so threading should be save. I also run my MCP4651-main()-code which works as before. In the next days I should receive my Raspberry Pi 2. As it has four cores real concurrency happens. So I will retest it with the new hardware. But since the tests run already in a multithreaded environment I expect no surprises.\r\n\r\nA last word on I2CBusImplTest: Testing concurrency uses Thread.sleep(…) to fake different situations. If the computer running the tests is busy by any other task those tests might fail. If the build is the only task the computer has to do everything works fine. I’m not happy about this but at the moment I have no idea how to improve this.\r\n\r\nStephan\r\n"},"comment":{"url":"https://api.github.com/repos/Pi4J/pi4j/issues/comments/82143876","html_url":"https://github.com/Pi4J/pi4j/pull/140#issuecomment-82143876","issue_url":"https://api.github.com/repos/Pi4J/pi4j/issues/140","id":82143876,"user":{"login":"RasPelikan","id":10763265,"avatar_url":"https://avatars.githubusercontent.com/u/10763265?v=3","gravatar_id":"","url":"https://api.github.com/users/RasPelikan","html_url":"https://github.com/RasPelikan","followers_url":"https://api.github.com/users/RasPelikan/followers","following_url":"https://api.github.com/users/RasPelikan/following{/other_user}","gists_url":"https://api.github.com/users/RasPelikan/gists{/gist_id}","starred_url":"https://api.github.com/users/RasPelikan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RasPelikan/subscriptions","organizations_url":"https://api.github.com/users/RasPelikan/orgs","repos_url":"https://api.github.com/users/RasPelikan/repos","events_url":"https://api.github.com/users/RasPelikan/events{/privacy}","received_events_url":"https://api.github.com/users/RasPelikan/received_events","type":"User","site_admin":false},"created_at":"2015-03-17T06:26:47Z","updated_at":"2015-03-17T06:26:47Z","body":"Hi Robert,\r\n\r\nI'm very sorry, but there is a bug: The method I2CBusImpl#getBus(...) takes an instance candidate which is used if there is no bus for the required bus-number available yet. This technique is used to hide all constructors entirely. I added this code recently and did not see that the method I2C#i2cOpen is called by the constructor. To clean this up move the lines of I2C#i2cOpen into a private method \"open()\" which has to be called by I2CBusImpl#getBus(...) if the instance candidate is used.\r\n\r\nAdditionally the timeout-parameters of the method I2CBusImpl#getBus(...) can be removed because the instance candidate is of a class derived from I2CBusImpl and so those values can be fetched by accessing the timeout-properties of the instance candidate directly.\r\n\r\nSorry for pushing buggy code,\r\nStephan"}}