timofurrer / w1thermsensor

A Python package and CLI tool to work with w1 temperature sensors like DS1822, DS18S20 & DS18B20 on the Raspberry Pi, Beagle Bone and other devices.
MIT License
493 stars 113 forks source link

Make finding sensor more robust after reboot #5

Closed rcpeters closed 9 years ago

rcpeters commented 9 years ago

After a reboot first time loading module fails to init, when the second time it would run. Added some check and retry to make sure the module inits correctly.

timofurrer commented 9 years ago

Thank you very much for the pull request! :beers: I've made some comments And unfortunately you broke the build...

Thanks !

rcpeters commented 9 years ago

Updated. Not sure what "magic int constants" means exactly as I don't normally use python. I assumed you wanted hard coded values to be declared as an class attribute. However I'm also not sure if you where referencing not using =+ augmented method.

timofurrer commented 9 years ago

There are some more issues I want to have fixed. Cl build passed again - very nice! Yes, with "magic int constants" I've ment the fact that you've just used 10 as a "magic value". With making it a class-member you've gave it a little description - more readability

timofurrer commented 9 years ago

Some other question: Why do you need this timeout for waiting for your sensor? This never happend to me. Are you facing an issue where the kernel modules are not loaded fast enough to create the slave files under /sys or just when you use the sensor directly after boot?

rcpeters commented 9 years ago

There are two issues I've fixed.

  1. The w1thermsensor loads the modules after trying access the sensors. Which means it's impossible for the code to work on systems that do not have the module loaded. Clearly it's been a while since any manual testing has been done.
  2. When a system command to load a module returns it doesn't guarantee the module is ready to be used. On my systems a Raspberry Pi B+ the directory or the sensors are not always immediately there. I chalk this up to new hardware threading differently. Hence rewriting the code the to be more robust is required.
timofurrer commented 9 years ago

Okay, cool! But the LOAD_KERNEL_MODULES member is needed for testing on machines where this kernel module is not available. There I do not want to try to load the modules.

rcpeters commented 9 years ago

Are you sure? If the path W1ThermSensor.BASE_DIRECTORY exist it's doesn't try to load the module.

timofurrer commented 9 years ago

That's true, but have you seen the tests? The BASE_DIRECTORY is mocked there and exists. And you can test without to be superuser, because modprobe on most linux systems need superuser permissions.

rcpeters commented 9 years ago

Yes I also modified the test and removed LOAD_KERNEL_MODULES. The test still work without super user permissions.

   make tests
   nosetests -v -s --with-coverage --cover-erase --cover-inclusive test --cover-package=w1thermsensor
   test_core.test_get_available_sensors_no_sensors ... ok
   test_core.test_get_available_sensors ... ok
   test_core.test_get_available_ds18s20_sensors ... ok
   test_core.test_get_available_ds1822_sensors ... ok
   test_core.test_get_available_ds18b20_sensors ... ok
   test_core.test_init_first_sensor ... ok
   test_core.test_init_first_sensor_of_specific_type ... ok
   test_core.test_init_specific_sensor ... ok
   test_core.test_sensor_temperature_in_C ... ok
   test_core.test_sensor_temperature_in_F ... ok
   test_core.test_sensor_temperature_in_K ... ok
   test_core.test_sensor_all_temperature_units ... ok
   test_core.test_sensor_type_name ... ok
   test_core.test_no_sensor_found_error ... ok
   test_core.test_sensor_not_ready_error ... ok
   test_core.test_unsupported_unit_error ... ok

   Name            Stmts   Miss  Cover   Missing
   ---------------------------------------------
   w1thermsensor      96      5    95%   60, 63-64, 143-144
   ----------------------------------------------------------------------
   Ran 16 tests in 2.106s

   OK
timofurrer commented 9 years ago

Okay, I haven't seen the line if not path.isdir(W1ThermSensor.BASE_DIRECTORY):

I will merge it. Thank's for contribution! :beers: