3
\$\begingroup\$

I have written a driver in JavaScript that decodes the payload of the sensor which is hexadecimal encoded. It receives the payload and the port details to the pre-defined function 'decode' which is the body of the driver provided by one of the IoT platforms and the driver has to be developed with this as an entry point and cannot be deleted.

The sensor I am using sends a payload which contains same kind of measurement (temperature, depth, signal strength) measured 4 times.(the values might change).

In my driver I have written the logic for 4 measurements all the four times. When the logic is still same. Instead I wan't to write it once and retrieve part of payload for every single measurement.

'1000000001121B7701131BAA01121BA90114F274' this is how the sample payload looks and it contains 4 measurements.

The ideal situation would be a function say measurement which calculates temperature, depth, etc., divides my payload into 4 parts and then uses it with functions for each sub payload rather than writing the whole measurement function four times. The current code works but is far from perfect.

Can anyone help? On how to improve the use of functions?

/* Arguments:
    * 'payload' - an array of bytes, raw payload from sensor,
    * 'port' - an integer value in range [0, 255], f_port value from sensor*/  

 /* diver for measurement sensor payload, one complete payload*/

function decode(payload, port) {
     var result=new Object();
     var payloadType = {};
     var Temperature = {}; 
     var Level = {};
     var Status = {}; 
     var Alarm = {};
     var SRCSRSSI = {};
     var Temperature1 = {};
     var Temperature2 = {};
     var Temperature3 = {};
     var Level1 = {};
     var Level2 = {};
     var Level3 = {};
     var SRCSRSSI1 = {};
     var SRCSRSSI2 = {};
     var SRCSRSSI3 = {};

    // check if its right device
    if (payload[1] != 00) {
    console.log(" Not TEK 766 payload");
    }else {
    // correct sensor payload type 
    console.log("TEK 766 payload")
    }

    // defines the payload type is measurement
    if (payload[0] == 16) {
        console.log("valid payload, Measurement");
        result.payloadType = 'Measurement';

        //measurement 1 starts here
        var level1 = payload[4]; //Defines the ullage reading in cms
        var level2 = payload[5]; //Defines the ullage reading in cms
        var temp1 = payload[6]; //Defines the temperature in °C
        result.Level = ((level1 * 256) + level2);
        if (temp1 < 32) { 
            result.Temperature = temp1;
        }else {
            result.Temperature = (temp1 - 256);
            }           
        if (payload[2] != 0){

        // payload describes alarm
        console.log("Alarm Limit Reached");
        var lim = (parseInt(payload[2], 16).toString(2))
        lim = lim.slice(4); 
        var x = (lim&1) ? "Alarm Reached Level-1":"Alarm Not-ReachedReached Level-1"
        console.log(x);
        result.Alarm = x;
        var y = (lim&2&3) ? "Alarm Reached Level-2":"Alarm Not-Reached Level-2";
        console.log(y);
        result.Alarm = y;
        var z = (lim&4&5&6&7) ? "Alarm Reached Level-3":"Not Reached Level-3";
        console.log(z); 
        result.Alarm = z;
        }else {
            result.Alarm = 'Alarm Level Not-Reached';
        }       
        //defines SRC major nibble
        var src = payload[7]; 
        var srssi = payload[7];
        result.SRCSRSSI = (src|srssi);

        //measurement 2 starts here 
        var level11 = payload[8]; //Defines the ullage reading in cms
        var level12 = payload[9]; //Defines the ullage reading in cms
        var temp11 = payload[10]; //Defines the temperature in °C
        result.Level1 = ((level11 * 256) + level12);
        if (temp11 < 32) { 
            result.Temperature1 = temp11;
        }else {
            result.Temperature1 = (temp11 - 256);
            }           

        //defines SRC major nibble
        var src1 = payload[11]; 
        var srssi1 = payload[11];
        result.SRCSRSSI1 = (src1|srssi1);

        //measurement3 starts here
        var level13 = payload[12]; //Defines the ullage reading in cms
        var level14 = payload[13]; //Defines the ullage reading in cms
        var temp12 = payload[14]; //Defines the temperature in °C
        result.Level2 = ((level13 * 256) + level14);
        if (temp12 < 32) {
            result.Temperature2 = temp12;
        }else {
            result.Temperature2 = (temp12 - 256);
            }           

        //defines SRC major nibble
        var src2 = payload[15]; 
        var srssi2 = payload[15];
        result.SRCSRSSI2 = (src2|srssi2);

        //measurement 4 starts here
        var level15 = payload[16]; //Defines the ullage reading in cms
        var level16 = payload[17]; //Defines the ullage reading in cms
        var temp13 = payload[18]; //Defines the temperature in °C
        result.Level3 = ((level15 * 256) + level16);
        if (temp13 < 32) { 
            result.Temperature3 = temp13;
        }else {
            result.Temperature3 = (temp13 - 256);
            }           

        //defines SRC major nibble
        var src3 = payload[19]; 
        var srssi3 = payload[19];
        result.SRCSRSSI3 = (src3|srssi3);

        return {
        "result": result,
        "payload": payload,
        "port": port
        }
    }
}

This code runs on an IoT platform named thingsHub which offers middleware services.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ A first approach would be to indent the code correctly and also remove the apostrophes at the beginning of the code. \$\endgroup\$
    – zomega
    Aug 11, 2019 at 15:22
  • 2
    \$\begingroup\$ I have read the instructions before posting the question. And I have done changes as to my understanding. If there is something specific please point it. This is my first time posting a query. \$\endgroup\$
    – Hemanth
    Aug 11, 2019 at 21:11
  • \$\begingroup\$ I'm trying to write one function say 'measurement' which calculates temperature, depth.. Divide my payload into 4 parts and then use it with function for each sub payload rather than writing the whole of measurement function four times. Any suggestions on how I can achieve this. \$\endgroup\$
    – Hemanth
    Aug 13, 2019 at 9:50
  • \$\begingroup\$ On what kind of platform does this run? Proper PC or embedded platform like an Edison? \$\endgroup\$
    – Mast
    Aug 14, 2019 at 9:38
  • 2
    \$\begingroup\$ This question is being discussed on meta \$\endgroup\$
    – Mast
    Aug 14, 2019 at 9:48

2 Answers 2

2
\$\begingroup\$

From a short review;

  • There are 16 unused variables in your code
  • In production code, don't call console.log, or if you have to because the code runs embedded, use a log function with a severity indication so that you can turn off or reduce logging
  • You are missing a ton of semicolons
  • You should jshint.com
  • You access payload[ with magical numbers, you should use well named constants instead
  • You should have lowercase properties, so result.Level -> result.level because that is more idiomatic
  • You should properly indent your code, you dont indent after if (payload[2] != 0){ for example, you can use a beautifier
  • This does not make sense to me : (lim&2&3) since 2 is 10 and 3 is 11, isn't that check equivalent to (lim&3)? If you can run ECMA6, I would consider binary AND'ing with properly named constants in binary format.
  • This makes even less sense;

    //defines SRC major nibble
    var src = payload[7]; 
    var srssi = payload[7];
    result.SRCSRSSI = (src|srssi);
    

    if payload[7] contains a byte per the initial comment, and you assign that exact same value to both src and srssi, then src|srssi will always be payload[7]. So you can you can replace that code with

    result.SRCSRSSI = payload[srssi]; //With srssi being a constant of value 7
    

    However, if you truly wanted to access the major nibble, then you should figure out what a 'major` nibble is because Google does not know. Then you can use this code to access either nibble in that byte.

  • This also worries me, I am not sure where this is running, but..

    if (temp13 < 32) { 
        result.Temperature3 = temp13;
    }else {
        result.Temperature3 = (temp13 - 256);
    }    
    

    Unless this runs on the moon or Mars, it seems more likely that your sensor detects a temperature of 33 degrees than -223 degrees. I would re-read the documentation of the sensor.

    Furthermore because you copy pasted that code, you could just put it in a function that called be twice.

  • I like the commenting in your code, I think it goes over the top only once;

    // check if its right device
    if (payload[1] != 00) {
      console.log(" Not TEK 766 payload");
    }else {
      // correct sensor payload type <- That comment was too much
      console.log("TEK 766 payload")
    }
    

    I would go for

    //Check if it is the right device
    console.log((payload[DEVICE]?"":"Not") + "TEK 766 payload");
    
\$\endgroup\$
2
  • \$\begingroup\$ Yes I realized that 15 unused variables are not doing anything. I already have one open object where I'm storing all my values. I have rectified that. The temperature logic I have used is as described in the documentation of the device, I suppose its conversion from Fahrenheit to Celcius. \$\endgroup\$
    – Hemanth
    Aug 14, 2019 at 16:33
  • \$\begingroup\$ Definitely not celcius -> fahrenheit conversion, body temperature in fahrenheit is 98.6°F, not -219°F \$\endgroup\$
    – konijn
    Aug 19, 2019 at 15:07
2
\$\begingroup\$

I see some discrepancies:

  • you say the payload is an array of bytes, yet the sample payload is a string;
  • you say the payload is in hexadecimal format, yet the tests in code suggest otherwise.

But I take it the crux of the matter is doing the same thing manually over and over again.

Let's say the payload is indeed an array of hex values and you want to work with numbers, then you could use your parseInt function for the whole shebang (assuming ES2015 availability):

const hex2Int = hex => parseInt(hex, 16)

and for clarity's sake it's nice to give names to things instead of referring to them by their position:

const [type, device, alarm, , ...measurements] = payload.map(hex2Int)

Now the measurements is an array containing all of the measurements (using rest parameter syntax), but it would be nicer to work with them individually. One could perhaps device a function which would chunk them up, but assuming we already know the length and amount of them, we could just hard-code them:

const chunked = [ measurements.slice(0, 4), measurements.slice(4, 8) /* etc. */ ]

So now one would need to decode one individual chunk. If the return value can be different from the original post, it would be nice if the result was an array of objects.

const decodeMeasurement = (acc, [ullage1, ullage2, temp, src]) =>
  acc.concat({
    level: ullage2 + ullage1 * 256
    , temperature: temp < 32 ? temp : temp - 256
    , SRCSRSSI: src | src
  })

so that one can chunked.reduce(decodeMeasurement, []) into an array.

But back to reviewing. If you don't care about non-measurement payloads, or non-target-device payloads, you should just return early.

For performance, you could consider using typed arrays.

\$\endgroup\$
2
  • \$\begingroup\$ 'yet the sample payload is a string', I put it in single quotes to highlight it. It is still received as array of bytes one of the reason to call them by their position. \$\endgroup\$
    – Hemanth
    Aug 14, 2019 at 15:57
  • \$\begingroup\$ The payload is indeed hexEncoded and contains not only decimal but also binary values. \$\endgroup\$
    – Hemanth
    Aug 14, 2019 at 16:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.