-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fixes a whole bunch of issues #12
base: master
Are you sure you want to change the base?
Conversation
…ut so it doesnt hang forever. cleaned up documentation.
@psy0rz are you using this in conjunction with the instathings ecosystem, or are you running this locally? I'm trying to find something that will convert modbus to mqtt messages and map the registers to objects using config data. As far as I can tell, this is exactly what I'm looking for. Is my understanding correct? |
Yes, i'm using it standalone. And yes: If there exists a herdsman-converter for your device, it will regulary poll the device and convert it to nice usabe json messages via mqtt. (https://github.com/Instathings/modbus-herdsman-converters/tree/master/devices) note that due to a bug you can configure it to poll just one modbus device. (issue #13 ) |
Thanks for the confirmation. Did you set up your own mongoDB? I've never set one up and there isn't a docker container built for my 32 bit pi so I'm not sure how much effort that will be or what it'll entail. I've got some holiday time this week, I'm going to try setting it up and adding some new herdsmen devices! |
I'm always using influxdb + grafana for stats and to make cool dashboards. |
Have you looked into using a worker? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looking great! 9/10 would merge. (Can't, unfortunately...)
switch (topic) { | ||
case `${baseTopic}/configure/set`: { | ||
const parsedMessage = JSON.parse(message); | ||
// console.log(parsedMessage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be debug or removed?
@@ -18,19 +19,30 @@ module.exports = async function start() { | |||
const { devices, modbus } = settings.get(); | |||
const options = { | |||
baudRate: modbus.baud_rate, | |||
parity: modbus.parity ? modbus.parity : 'none', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like defaults are handled differently elsewhere. Also, couldn't this be shorter with modbus.parity || 'none'
? I don't remember how that handled 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind this, would go wrong with handling 0.
}; | ||
await this.modbus.connectRTUBuffered(modbus.port, options); | ||
this.modbus.setTimeout(1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be awaited? And potentially a setting? (I know, I also made a PR with a magic number here O:) )
this.mqtt = mqtt; | ||
this.modbus = modbus; | ||
this.id = device.id; | ||
this.model = device.model; | ||
this.modbusId = device.modbus_id; | ||
this.descriptor = modbusHerdsmanConverters.findByModbusModel(this.model); | ||
if (this.descriptor === undefined) throw (Error(`Device ${device.id} has unkown model: ${this.model}`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES PLEASE! Would've saved me two hours yesterday....
let value; | ||
try { | ||
value = await this.modbus.readInputRegisters(address, 1); | ||
logger.debug(`Polling modbus device ${this.id}: modbusId=${this.modbusId}, fc=${fc}, address=${address}, readlen=${readlen}`); | ||
switch (fc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/yaacov/node-modbus-serial/tree/a0a020fa6c0888ffa0b60a845e70b4dfc14d14ec/worker, could leave this switch case to that library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially add "type" to the address descriptor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm also a bit confused about the readlen parameter. It seems that it's always doubled? Readlen of 4 gives me a buffer of 8, readlen 2 a buffer of 4, and readlen 1 a buffer of 2. Maybe describe that somewhere, if that's indeed how it's supposed to work?
Right, modbus works with 16-bit registers...
} catch (err) { | ||
logger.error(err); | ||
logger.error(`While reading modbus: ${err.toString()}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modbus2mqtt:error 2022-01-15 09:35:05: While reading modbus: [object Object]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using JSON.stringify instead of .toString gives:
modbus2mqtt:error 2022-01-15 09:37:26: While reading modbus: {"name":"TransactionTimedOutError","message":"Timed out","errno":"ETIMEDOUT"}
@@ -62,7 +85,7 @@ class Modbus extends events.EventEmitter { | |||
const payload = JSON.stringify(result); | |||
this.mqtt.publish(topic, payload); | |||
} catch (err) { | |||
logger.error(err); | |||
logger.error(`While handeling modbus: ${err.toString()}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handling
} catch (err) { | ||
logger.error(err); | ||
logger.error(`While reading modbus: ${err.toString()}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using JSON.stringify instead of .toString gives:
modbus2mqtt:error 2022-01-15 09:37:26: While reading modbus: {"name":"TransactionTimedOutError","message":"Timed out","errno":"ETIMEDOUT"}
let value; | ||
try { | ||
value = await this.modbus.readInputRegisters(address, 1); | ||
logger.debug(`Polling modbus device ${this.id}: modbusId=${this.modbusId}, fc=${fc}, address=${address}, readlen=${readlen}`); | ||
switch (fc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm also a bit confused about the readlen parameter. It seems that it's always doubled? Readlen of 4 gives me a buffer of 8, readlen 2 a buffer of 4, and readlen 1 a buffer of 2. Maybe describe that somewhere, if that's indeed how it's supposed to work?
Right, modbus works with 16-bit registers...
Its a xmas miracle! \o/
Fixed debugging, logging, exception handling and documentation.
Added support for multiple modbus function codes and read lengths.
Added parity/databits/stopbits settings.