Add bambulab to latest

[DutchmanNL]

Please add my adapter ioBroker.bambulab to latest.

This pull request was created by https://www.iobroker.dev c0726ff.

[github-actions]

Automated adapter checker

ioBroker.bambulab

👍 No errors found

• 👀 [W171] "common.title" is deprecated in io-package.json
• 👀 [W400] Cannot find "bambulab" in latest repository

Add comment "RE-CHECK!" to start check anew

[mcm1957]

First of all - THANK YOU for the time and effort you spend to maintain this adapter.

I would like to give some (preliminary) feedback based on my personal oppinion. This is NOT an offical review and @Apollon77 might have several additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him (or wait for a response from him) if you cannot follow my suggestsions or before you spend major effort.

• sensitive information, especially passwords should be stored encrypted

  You configuration seems to contain passwords (or other sensitive information) which is not stored encrypted. As long as those information is not stored inside a table you can encrypt and secure this infomation simply by adding the following configuration to io-package.json. The iob runtime will encrypt / decrypt the data as required; no addition programming effort is required. Note that users will need to reenter this info one time after adding encryption, so drop a note at release notes.

  "encryptedNative": [
    "password",
    "data2"
  ],
  "protectedNative": [
    "password",
    "data2"
  ],
  

  You use encryptedNative but not protectedNative. I just want to note it; if its not on purpose consider adding it.

• review writeable states (button states)

  If noticed that at 'controlStates' the buttons 'chamberLight' and 'start' have an attribut 'write:true' while 'stop' and 'resume' do not have it. This looks inconsistent but might be OK depending on other code or your goals. So take it as info only.

• reevaluate state roles

  Only the values specified here (https://github.com/ioBroker/ioBroker.docs/blob/master/docs/en/dev/stateroles.md) may be used as state roles. Do not invent new names as this might disturb the functionalyity of other adapters or vis.

  In addition the roles MUST match the read/write functionality. As an example a role value.* requires that the state is a readable state. Input states (write only) should have roles like level.*. Please read the description carefully.

  For your adapter I want to note especially:

  • button.resume is not (yet ?) listed there. It might be a good idea to request an addition befor using it (eventually discuss with Apollon77 / bluefox directly)
  • I did not install the adapter, but check if button states have read=false attribute set. Looks like you do not specify read and I think read defaults to true - but not sure for this.

• some labels seem to have no translation into all languages

  It seems that some labels are not translated into all supported languages, i.e. de/translations.json
  Please consider adding missing translations

{
	"bambulab adapter settings": "Adaptereinstellungen für bambulab",
	"option1": "Option 1",
	"option2": "Option 2"
}

• consider using adapter.setTimeout / this.setTimeout instead of (standard) setTimeout

  The adapter package provides wrapper routines for native setTimeout, setInterval, clearTimeout and clearInterval. Using those routines ensures that timers are cancelled on on load. Additional checks on thomse limits might be performed, too. So consider replacing native setTimeout/clearTimeout by adapter.setTimeout/adapter.clearTimeout or this.setTimeout/this.clearTimeout. The same refers to setInterval/clearInterval.

• check and limit configurable timeouts / intervals

  Node setTimeout/setInterval routines have a maximum allowed value of 2,147,483,647 ms. Using delays larger than 2,147,483,647 ms (about 24.8 days) result in the timeout being executed immediately. So all (user configurable) values passed to setTimeout / setInterval should be checked and limited.

• check and adapt testing

  Standard iobroker testing is required but seems to be missing. Please add and setup ./gizthub/workflows/test-and-release.yml

• adapt testing to supported node releases

  Consider adding node 20 tests unless you already know incompatibilities.

  So the recommended testmatrix is [16.x, 18.x, 20.x],

Thanks for reading and evalutaing this suggestions.
McM1957

[DutchmanNL]

First of all - THANK YOU for the time and effort you spend to maintain this adapter.

Thank you for your swift and constrictive review :)!

I would like to give some (preliminary) feedback based on my personal oppinion. This is NOT an offical review and @Apollon77 might have several additional suggestions or even a different oppinion to one or the other statement. Please feel free to contact him (or wait for a response from him) if you cannot follow my suggestsions or before you spend major effort.

appreciated!

• sensitive information, especially passwords should be stored encrypted

  You use encryptedNative but not protectedNative. I just want to note it; if its not on purpose consider adding it.

Thank you, solved
DrozmotiX/ioBroker.bambulab@5a3a619

• review writeable states (button states)
  If noticed that at 'controlStates' the buttons 'chamberLight' and 'start' have an attribut 'write:true' while 'stop' and 'resume' do not have it. This looks inconsistent but might be OK depending on other code or your goals. So take it as info only.

Thank you, solved DrozmotiX/ioBroker.bambulab@0fedf4e

• reevaluate state roles

  • button.resume is not (yet ?) listed there. It might be a good idea to request an addition befor using it (eventually discuss with Apollon77 / bluefox directly)

for now I removed button.resume and looked just button. Resume make sense in generic as several controls, but mostly a combination of play/pause, know the command resume

• I did not install the adapter, but check if button states have read=false attribute set. Looks like you do not specify read and I think read defaults to true - but not sure for this.

added read: true just to be sure DrozmotiX/ioBroker.bambulab@67c8cc7

• some labels seem to have no translation into all languages

translations added DrozmotiX/ioBroker.bambulab@c4030ed

• consider using adapter.setTimeout / this.setTimeout instead of (standard) setTimeout

will evaluate this new function in a later version, for now and latest release current implementation with set timeout and clear (also in unload) should be sufficient

• check and limit configurable timeouts / intervals
  Node setTimeout/setInterval routines have a maximum allowed value of 2,147,483,647 ms. Using delays larger than

timeout has a fixed value and is ensured to be cleaned before fun

				if (timeout) {clearTimeout(timeout); timeout = null;}
				timeout = setTimeout(async function () {
					client.reconnect();
				}, 

• check and adapt testing
  Standard iobroker testing is required but seems to be missing. Please add and setup ./gizthub/workflows/test-and-release.yml

workflow is present https://github.com/DrozmotiX/ioBroker.bambulab/blob/main/.github/workflows/test-and-release.yml

• adapt testing to supported node releases
  Consider adding node 20 tests unless you already know incompatibilities.
  So the recommended testmatrix is [16.x, 18.x, 20.x],

testing for node 20 added DrozmotiX/ioBroker.bambulab@e46638e

Thanks for reading and evalutaing this suggestions. McM1957

thank you reviewing & al valuable feedback !

[mcm1957]

Hi and thanks for the fast response.
Sorry for some issues at the end of my review - I simply forgot to remove them from the template (i.e. limiting timout values).

BUT

start : {
	name: 'Start printing',
	type: 'boolean',
	role: 'button.start',
	read: true,
	write: true

should have set read:false according to documentation:
'common.type=boolean, common.write=true, common.read=false'
https://github.com/ioBroker/ioBroker.docs/blob/master/docs/en/dev/stateroles.md#buttons-booleans-write-only

Sorry that my first statement was misleading.

If you feel, this is a documentation error, please check with @Apollon77 or othgers from core team and create a correction PR / issue at documentation.

[mcm1957]

FYI:
I've created a PR to add button.resume.

Lets wait if it will be accepted.
ioBroker/ioBroker.docs#429

[DutchmanNL]

Hi and thanks for the fast response. Sorry for some issues at the end of my review - I simply forgot to remove them from the template (i.e. limiting timout values).

BUT

start : {
	name: 'Start printing',
	type: 'boolean',
	role: 'button.start',
	read: true,
	write: true

should have set read:false according to documentation: 'common.type=boolean, common.write=true, common.read=false' https://github.com/ioBroker/ioBroker.docs/blob/master/docs/en/dev/stateroles.md#buttons-booleans-write-only

Sorry that my first statement was misleading.

If you feel, this is a documentation error, please check with @Apollon77 or othgers from core team and create a correction PR / issue at documentation.

@Apollon77

Can you please provide your feedback here ? I was in the impression the read flag should be always default true as it makes otherwise no sense you can always read a state ?

[Apollon77]

Can you please provide your feedback here ? I was in the impression the read flag should be always default true as it makes otherwise no sense you can always read a state ?

This is the usual "formal" discussion.

A Button ( "Taster" only triggers when you press but else have no state), so should also be not readable .... This is also like HomeMatic does it ... a Switch has clear states true or false and so can be read.

[DutchmanNL]

Can you please provide your feedback here ? I was in the impression the read flag should be always default true as it makes otherwise no sense you can always read a state ?

This is the usual "formal" discussion.

A Button ( "Taster" only triggers when you press but else have no state), so should also be not readable .... This is also like HomeMatic does it ... a Switch has clear states true or false and so can be read.

with that explanation it makes sense, I changed my lines to ensure all buttons have the read flag set to false DrozmotiX/ioBroker.bambulab@177f176

[mcm1957]

@DutchmanNL

This adapter has been released to latest repository and should be visible within 24h maximum.

Please create a thread at https://forum.iobroker.net/category/91/tester titled like "Test Adapter " to collect some user feedback and provide a link to this topic when requesting addition to stable repository later.

Note: If an other testing topic already exists, ist OK to continue using this topic too.

[DutchmanNL]

@mcm1957 thank you, forum thread already exists 😉

https://forum.iobroker.net/topic/67178/bambulab-3d-drucker-adapter?_=1692616355318

[mcm1957]

Its fully ok to keep this thread.
I only want to e nsure in future taht every dev creates a forum thread at least after going to latest so that one (i ?) can make a rough scan whether the adapter has serios problems when first going to stable. Ok, some mor work :-) - but I do not think that such a "suggestions" is not ok. I did not filter users when merging PRs today :-).