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

Roborock/Capability Carpet Mode #661

Merged
merged 12 commits into from
Feb 8, 2021
Merged

Roborock/Capability Carpet Mode #661

merged 12 commits into from
Feb 8, 2021

Conversation

bensweet86
Copy link
Contributor

Carpet mode capability for Roborock vacuums. Includes

  • Capability router with get / put for managing settings & enabled status
  • ValetudoCarpetModeConfiguration entity to store configuration through serializable entity
  • Capability across roborock vacuums, tested with Gen1

@Hypfer
Copy link
Owner

Hypfer commented Jan 22, 2021

As mentioned in the Telegram group, I'd like to omit the additional parameters and instead just have a simple boolean toggle like the PersistentMapControlCapability, since those parameters aren't documented and most vendors don't even allow to edit them at all

@bensweet86
Copy link
Contributor Author

I tested the capability of Roborock to handle only the enabled status, issue is that set_carpet_mode requires the other parameters as if enabled is the only parameter toggled, the other parameters such as stall time and current low / high / integral are set to 0 when not supplied.
This implements the complete functionality, otherwise would require the workaround as part of the config application to the vacuum:

  • get_carpet_mode to get the list of these parameters
  • set_carpet_mode would initially need to get_carpet_mode to get the current values, overwriting the enabled status with the UI value

If we still need to include get as part of the set process why not have the capability to see and potentially edit these parameters?
It wouldn't take much to allow a vacuum with the parameters to toggle vs another one with just an enabled flag to be supported with parts of the API endpoints optional.

@Hypfer
Copy link
Owner

Hypfer commented Jan 22, 2021

set_carpet_mode requires the other parameters

I'd simply hard-code the default values for set_carpet_mode since they're default for a reason

why not have the capability to see and potentially edit these parameters?

Because it's a generic capability + capabilityRouter and it makes no sense for it to take roborock-specific arguments.

The overall goal of the rewrite was to be less vendor-specific which also means that not everything that would in theory be possible will also be implemented in Valetudo.

The whole idea being that you'll get mostly the same experience, no matter what robot Valetudo is running on.
(Minus polyfilling of course since that can become very complicated very fast)

If there are vendor-specific configuration options, reasonable defaults which fit 80% of the users should be used.




I appreciate your contributions, however maybe you'd be better off using Valetudo RE since that project is focussed more on supporting more vendor-specific details.

It also already supports map backup&restore which is very useful for the roborock v1 but doesn't really make sense to implement in Valetudo due to the market having moved forward from that and it being highly unlikely that we'll see non-persistent-map vacuum robots being released in the future

@bensweet86
Copy link
Contributor Author

Don't get me wrong, I have never had a need to customise these parameters so driven to include. If I take the parameters hardcoded in the UI as the defaults, these were different to the parameters that were in my Gen1 out of the box. Perhaps then the best option is to - as part of the Roborock capability - would be to stick with the get / set instead of hardcoding to ensure we aren't changing a value unintentionally.

@Hypfer
Copy link
Owner

Hypfer commented Jan 22, 2021

Does the firmware reject the command if it is sent without parameters? 🤔

Another approach could be to extend the Capability's contructor to accept the default values for this model similar to this:

class DreameBasicControlCapability extends BasicControlCapability {
/**
* The most basic functionalities
*
* @param {object} options
* @param {import("../../../core/ValetudoRobot")|any} options.robot
*
* @param {object} options.miot_actions
* @param {object} options.miot_actions.start
* @param {number} options.miot_actions.start.siid
* @param {number} options.miot_actions.start.aiid
*
* @param {object} options.miot_actions.stop
* @param {number} options.miot_actions.stop.siid
* @param {number} options.miot_actions.stop.aiid
*
* @param {object} options.miot_actions.pause
* @param {number} options.miot_actions.pause.siid
* @param {number} options.miot_actions.pause.aiid
*
* @param {object} options.miot_actions.home
* @param {number} options.miot_actions.home.siid
* @param {number} options.miot_actions.home.aiid
*/
constructor(options) {
super(options);
this.miot_actions = options.miot_actions;
}
async start() {
await this.robot.sendCommand("action",
{
did: this.robot.deviceId,
siid: this.miot_actions.start.siid,
aiid: this.miot_actions.start.aiid,
in: []
}
);

and then providing those inside the Vacuum implementation like that

this.registerCapability(new capabilities.DreameBasicControlCapability({
robot: this,
miot_actions: {
start: {
siid: MIOT_SERVICES.VACUUM_1.SIID,
aiid: MIOT_SERVICES.VACUUM_1.ACTIONS.RESUME.AIID
},
stop: {
siid: MIOT_SERVICES.VACUUM_2.SIID,
aiid: MIOT_SERVICES.VACUUM_2.ACTIONS.STOP.AIID
},
pause: {
siid: MIOT_SERVICES.VACUUM_1.SIID,
aiid: MIOT_SERVICES.VACUUM_1.ACTIONS.PAUSE.AIID
},
home: {
siid: MIOT_SERVICES.BATTERY.SIID,
aiid: MIOT_SERVICES.BATTERY.ACTIONS.START_CHARGE.AIID
}
}
}));

@bensweet86
Copy link
Contributor Author

Yes, I'm going down a path similar to what you noted above. Thanks for the pointer.

The Roborock API in this instance will actually accept the request if those additional parameters haven't been supplied, which is are then defaulted to 0. Only having a Gen1 and knowing this actually isn't supported I cannot say if this is normal or not, presume it is so we will have to handle it accordingly by requesting the vacuum state, toggling enabled then sending back the modified configuration in full.

@bensweet86
Copy link
Contributor Author

bensweet86 commented Jan 23, 2021

@Hypfer, changes updated. The status of carpet mode isn't emitted in the base Roborock state attributes so functionality reverted back to basis on / off toggle with no entity. Values set at this point from get / set sequence instead of defaults in the code.
What is the position for these state attributes such as this carpet mode and consumable state attribute where the state is only updated when the settings section loads the page? As below, provision in current code for dnd_enabled state from "get_status" request but carpet mode isn't returned within this request. Has there been any consideration for this, otherwise the state attribute for carpet mode will only exist upon settings change / page load as with consumables.
As far as DND below, we can probably remove this from the state attribute given we have the settings persisted out of the previously implemented DND capability and this get_status DND state reflects this same state; i.e. get_status DND = 1 if enabled, and doesn't reflect if DND is blocking operations (i.e. outside of DND timeframe this value is still 1).

@bensweet86
Copy link
Contributor Author

this.state.upsertFirstMatchingAttribute(new stateAttrs.PersistentMapSettingStateAttribute({
value: persistentMapSetting
}));
//data["dnd_enabled"]
//data["map_present"]

@bensweet86
Copy link
Contributor Author

consumables.forEach(c => this.robot.state.upsertFirstMatchingAttribute(c));
this.robot.emitStateUpdated();
return consumables;

@Hypfer Hypfer merged commit 54ca606 into Hypfer:master Feb 8, 2021
@bensweet86 bensweet86 deleted the CarpetMode-Capability branch February 9, 2021 11:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants