Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve DS1820 driver #1382

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Improve DS1820 driver #1382

wants to merge 3 commits into from

Conversation

Lenart12
Copy link

@Lenart12 Lenart12 commented Oct 6, 2024

New features:

  • Handle all types of DS1820 tempature sensors by detecting the family of the sensor and calculating the temperature accordingly
  • Clean up some log prints
  • Set the channel value with current temperature
  • Update the reading every second instead of every 15s

@MaxineMuster
Copy link
Contributor

Thanks for the extensions, will take a deeper look later.

I would not do a conversion every second, from my point of view it's kind of "waste of ressources", if there is no special use case with very fast changing temperatures (and even then the sensor won't be able to follw so fast).

But for sure its discussable - maybe simply add the interval as argument to the drivers call.

New features:
* Handle all types of DS1820 tempature sensors by detecting the family of the sensor and calculating the temperature accordingly
* Clean up some log prints
* Set the channel value with current temperature
* Set the period of the conversion interval with an optional first parameter
* Start the conversion instantly and not wait for multiple of the conversion period
@Lenart12
Copy link
Author

Lenart12 commented Oct 7, 2024

I added an optional argument to the driver as you said 👍
Also greatly simplified the discover family algorithm since (for now) its assumed for only 1 device on the bus anyways, and conversion now starts when the driver starts, it doesnt wait for a multiple of conversion period, so we can get the first reading faster after device boot :^)

- Output logging to `LOG_FEATURE_SENSOR` and not `CFG` as it would be expected
- Created a macro `DS1820_LOG` to simplify logging statements.
- Use the macro in various places throughout the driver
@MaxineMuster
Copy link
Contributor

Very good work.
Some more ideas:

  • remove the unused CRC code
  • print resolution in "clear text"
  • remove duplicate of "detected family"
diff --git a/src/driver/drv_ds1820_simple.c b/src/driver/drv_ds1820_simple.c
index c9998eda..4f4afa11 100644
--- a/src/driver/drv_ds1820_simple.c
+++ b/src/driver/drv_ds1820_simple.c
@@ -314,25 +314,6 @@ int OWTouchByte(int Pin, int data)
         return result;
 }
 
-
-
-
-uint8_t OWcrc( uint8_t *data, uint8_t len)
-{
-	uint8_t crc = 0;
-	
-	while (len--) {
-		uint8_t inb = *data++;
-		for (uint8_t i = 8; i; i--) {
-			uint8_t mix = (crc ^ inb) & 0x01;
-			crc >>= 1;
-			if (mix) crc ^= 0x8C;
-			inb >>= 1;
-		}
-	}
-	return crc;
-}
-
 // quicker CRC with lookup table
 // based on code found here: https://community.st.com/t5/stm32-mcus-security/use-stm32-crc-to-compare-ds18b20-crc/m-p/333749/highlight/true#M4690
 // Dallas 1-Wire CRC Test App -
@@ -397,7 +378,7 @@ int DS1820_DiscoverFamily() {
 	uint8_t family = ROM[0];
 	if (family == 0x10 || family == 0x28) {
 		ds18_family = family;
-		DS1820_LOG(INFO, "Discover Family %x", family);
+		DS1820_LOG(INFO, "Discover Family - discovered %x", family);
 		return 1;
 	} else {
 		DS1820_LOG(DEBUG, "Discover Family %x not supported", family);
@@ -427,7 +408,6 @@ void DS1820_OnEverySecond() {
 			{
 				scratchpad[i] = OWReadByte(Pin);//read Scratchpad Memory of DS
 			}
-//			crc= OWcrc(scratchpad, 8);
 			crc= Crc8CQuick(scratchpad, 8);
 			if (crc != scratchpad[8])
 			{
@@ -454,7 +434,7 @@ void DS1820_OnEverySecond() {
 					else if (cfg == 0x20) raw = raw & ~3; // 10 bit res, 187.5 ms
 					else if (cfg == 0x40) raw = raw & ~1; // 11 bit res, 375 ms
 					raw = raw << 3; // multiply by 8
-					DS1820_LOG(DEBUG, "family=%x, raw=%i, cfg=%x", ds18_family, raw, cfg);
+					DS1820_LOG(DEBUG, "family=%x, raw=%i, cfg=%x (%i bit resolution)", ds18_family, raw, cfg, 9+(cfg)/32) ;
 				}
 			
 				// Raw is t * 128
@@ -505,7 +485,8 @@ void DS1820_OnEverySecond() {
 						DS1820_LOG(ERROR, "Family not discovered");
 						return;
 					}
-					DS1820_LOG(INFO, "Family discovered %x", ds18_family);
+					// Same info is printed by DS1820_DiscoverFamily();
+					// DS1820_LOG(INFO, "Family discovered %x", ds18_family);
 				}
 
 				OWWriteByte(Pin,0xCC);

Co-authored-by: MaxineMuster <146550015+MaxineMuster@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants