Skip to content

Commit

Permalink
Merge pull request #363 from krkeegan/Modem_Delete
Browse files Browse the repository at this point in the history 
Simplify Modem DB Delete by Using DB_Flags
  • Loading branch information
@krkeegan
krkeegan authored Mar 18, 2021
2 parents 84dfe61 + 28f33a1 commit 139aa11
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 224 deletions.
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Revision Change History

## [0.8.1]

### Fixes

- Alters the link delete functionality on the Modem. Solves an issue where
some were not able to delete a link on the Modem. (thanks @zimmer62)
([PR 363][P363])

## [0.8.0]

This update is the culmination of a significant refactor of the underlying
Expand Down Expand Up @@ -625,3 +633,4 @@ will add new features.
[P354]: https://github.com/TD22057/insteon-mqtt/pull/354
[P355]: https://github.com/TD22057/insteon-mqtt/pull/355
[P359]: https://github.com/TD22057/insteon-mqtt/pull/359
[P363]: https://github.com/TD22057/insteon-mqtt/pull/363
121 changes: 7 additions & 114 deletions insteon_mqtt/db/Modem.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from .. import log
from .. import message as Msg
from .. import util
from ..CommandSeq import CommandSeq
from .ModemEntry import ModemEntry
from .DbDiff import DbDiff

Expand Down Expand Up @@ -367,20 +366,6 @@ def add_on_device(self, entry, on_done=None):
def delete_on_device(self, entry, on_done=None):
"""Delete an entry on the device.
This will delete ALL the entries for an address and group. The modem
doesn't support deleting a specific controller or responder entry -
it just deletes the first one that matches the address and group. To
avoid confusion about this, this method clears all relevant entries
from our local cache, then it searches the modem for only these
relevant entries and adds them back to our cache (this ensures that
our cache is correct as to these entries), then it deletes all of the
relevant entries on the modem, and finally it restores all related
entries that were not marked for deletion.
This complex process means that it no longer matters if the modem
database cache is accurate. And, while sounding complex, this only
takes a second or two to perform.
The on_done callback will be passed a success flag (True/False), a
string message about what happened, and the DeviceEntry that was
created (if success=True).
Expand All @@ -394,108 +379,16 @@ def delete_on_device(self, entry, on_done=None):
"""
on_done = util.make_callback(on_done)

seq = CommandSeq(self.device, "Modem db delete complete", on_done,
name="ModemDBDel")

# Find all the entries that match the addr and group inputs.
for del_entry in self.find_all(entry.addr, entry.group):
self.delete_entry(del_entry)

# db_flags = Msg.DbFlags.from_bytes(bytes(1))
db_flags = Msg.DbFlags(in_use=True, is_controller=entry.is_controller,
# Build the delete message. The Data1-3 values are always 0x00 as
# they are ignored by the modem.
db_flags = Msg.DbFlags(in_use=True,
is_controller=entry.is_controller,
is_last_rec=False)
msg = Msg.OutAllLinkUpdate(Msg.OutAllLinkUpdate.Cmd.EXISTS, db_flags,
entry.group, entry.addr, bytes(3))
msg_handler = handler.ModemDbSearch(self, on_done=on_done)

# Queue the search message
seq.add_msg(msg, msg_handler)

# Queue the post search function.
seq.add(self._delete_on_device_post_search, entry)

# Run the sequence
seq.run()

#-----------------------------------------------------------------------
def _delete_on_device_post_search(self, entry, on_done=None):
"""Delete a series of entries on the device.
This is performed as part of the delete_on_device() function, after
the search has been performed of the device.
Args:
addr: (Address) The address to delete.
group: (int) The group to delete.
on_done: Optional callback which will be called when the
command completes.
"""
on_done = util.make_callback(on_done)

# Find all the entries that match the addr and group inputs.
entries = self.find_all(entry.addr, entry.group)

# The modem will erase entries in the order it finds them - we can't
# erase a specific entry. So find the ctrl/resp entry we want and
# see if there is a different entry first - if there is call delete
# until we delete the one we actually want and then restore the other
# entry after we're done.
#
# In a proper database, we should only have found 1 or 2 entries but
# it's possible to push duplicate records into the db. So just find
# the first entry that matches our request and only restore one other
# which which be the opposite ctrl/responder flag version.
restore = None
erase_idx = None
for i in range(len(entries)):
if entries[i].is_controller == entry.is_controller:
erase_idx = i
break

if not restore:
restore = entries[i]

if erase_idx is None:
LOG.warning("Modem db Delete: Entry was not on modem.")
on_done(True, "Entry not on modem", None)
return

LOG.debug("Modem delete %d of %d entries", erase_idx + 1,
len(entries))

# Build the first delete message. The Handler will remove the
# entries from our db when it get's an ACK.
#
# Modem will only delete if we pass it an empty flags input (see the
# method docs). This deletes the first entry in the database that
# matches the inputs.
db_flags = Msg.DbFlags.from_bytes(bytes(1))
msg = Msg.OutAllLinkUpdate(Msg.OutAllLinkUpdate.Cmd.DELETE, db_flags,
entry.group, entry.addr, bytes(3))
msg_handler = handler.ModemDbModify(self, entries[0], on_done=on_done)

# Add the other delete calls to the handler - these will run in
# sequence as we get ACKs for each call. The final call will call
# the on_done() callback.
for i in range(1, erase_idx + 1):
msg_handler.add_update(msg, entries[i])

# Add a command to restore the entry we didn't want to delete.
if restore:
if restore.is_controller:
cmd = Msg.OutAllLinkUpdate.Cmd.ADD_CONTROLLER
else:
cmd = Msg.OutAllLinkUpdate.Cmd.ADD_RESPONDER

db_flags = Msg.DbFlags(in_use=True,
is_controller=restore.is_controller,
is_last_rec=False)
msg2 = Msg.OutAllLinkUpdate(cmd, db_flags, restore.group,
restore.addr, restore.data)
msg_handler.add_update(msg2, restore)

# Send the first message. If it ACK's, it will keep sending more
# deletes - one per entry.
msg_handler = handler.ModemDbModify(self, entry, on_done=on_done)

# Send the message.
self.device.send(msg, msg_handler)

#-----------------------------------------------------------------------
Expand Down
13 changes: 9 additions & 4 deletions insteon_mqtt/handler/ModemDbModify.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,15 @@ def msg_received(self, protocol, msg):

# If we get a NAK message, signal an error and stop.
if not msg.is_ack:
if msg.cmd == Msg.OutAllLinkUpdate.Cmd.DELETE or self.is_retry:
# We should never fail on a DELETE. If this is a retry then
# the matching add/update already failed, which also should not
# happen.
if msg.cmd == Msg.OutAllLinkUpdate.Cmd.DELETE:
# A failed delete only happens.
LOG.error("Modem db delete failed: %s", msg)
self.on_done(False, "Delete entry from Modem db failed, " +
"entry likely doesn't exist, try running " +
"`refresh modem`", self.entry)

elif self.is_retry:
# A failed retry, stop to prevent infinite looping
LOG.error("Modem db update failed: %s", msg)
self.on_done(False, "Write to Modem db failed, try running " +
"`refresh modem`", self.entry)
Expand Down
12 changes: 3 additions & 9 deletions insteon_mqtt/message/DbFlags.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,12 @@ def copy(self):
return DbFlags(self.in_use, self.is_controller, self.is_last_rec)

#-----------------------------------------------------------------------
def to_bytes(self, modem_delete=False):
def to_bytes(self):
"""Convert to bytes.
The inverse of this is DbFlags.from_bytes(). This is used to output
the flags as bytes.
Args:
is_delete (bool): Set to True if this is delete call to the modem
to modify the database.
Returns:
bytes: Returns a 1 byte array containing the bit flags.
"""
Expand All @@ -101,10 +97,8 @@ def to_bytes(self, modem_delete=False):

# Not sure why this is needed. Insteon docs say bit 5 is unused.
# But all the records have it set and if it's not set here, commands
# sent to modify the database will fail. But, for delete calls to
# the modem db, this must be zero.
if not modem_delete:
data |= 1 << 5
# sent to modify the database will fail.
data |= 1 << 5

return bytes([data])

Expand Down
17 changes: 8 additions & 9 deletions insteon_mqtt/message/OutAllLinkUpdate.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ class OutAllLinkUpdate(Base):
# The modem developers guide is wrong regarding much of this. There are
# no alternative commands such as 'add or modify' as stated in the
# document. All commands do one function only. Additionally, the entry
# matching includes the ctrl/resp status except on Delete(which requires
# the db_flags to be 0x00).
# matching includes the ctrl/resp status.
#
# The following is what each control code does based on testing.
# 0x00 - FIND FIRST - Searches for an entry matching addr, group, but not
Expand All @@ -47,10 +46,12 @@ class OutAllLinkUpdate(Base):
# Will produce a NACK if a responder record matching the addr,
# group, AND ctrl/resp already exists. Will also nack if the entry
# in the command is not a responder entry.
# 0x80 - DELETE - Searches for an entry matching addr, group, but not
# ctrl/resp, indeed the db_flags need to be 0x00 otherwise the
# search will NACK. The first matching entry (ctrl or resp) will
# be deleted. If no entry can be found a NACK will be returned.
# 0x80 - DELETE - Searches for an entry matching ctrl/resp, addr, and
# group, but ignores any values on Data1-3. Any matching entry will
# be deleted. The modem only allows a single ctrl/resp + group +
# addr entry. For example you cannot have multiple resp entries
# for the same group and addr with different data1-3 values.
# If no matching entry can be found a NACK will be returned.

# Valid command codes
class Cmd(enum.IntEnum):
Expand Down Expand Up @@ -125,9 +126,7 @@ def to_bytes(self):
"""
o = io.BytesIO()
o.write(bytes([0x02, self.msg_code, self.cmd.value]))
# db_flags must be 0x00 for a modem delete
o.write(self.db_flags.to_bytes(
modem_delete=self.cmd == self.Cmd.DELETE))
o.write(self.db_flags.to_bytes())
o.write(bytes([self.group]))
o.write(self.addr.to_bytes())
o.write(self.data)
Expand Down
101 changes: 14 additions & 87 deletions tests/db/test_Modem.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,98 +149,25 @@ def test_add_on_device_update_resp(self, test_device,
def test_delete_on_device_update_resp(self, test_device,
test_entry_dev1_resp,
test_entry_dev1_ctrl):
# add_on_device(self, entry, on_done=None)
# test calling delete to ensure that search happens
# test calling delete
test_device.add_entry(test_entry_dev1_resp)
test_device.add_entry(test_entry_dev1_ctrl)
assert len(test_device) == 2
with mock.patch.object(IM.CommandSeq, 'add_msg') as mocked:
test_device.delete_on_device(test_entry_dev1_resp)
assert len(test_device) == 0
assert mocked.call_count == 1
assert (mocked.call_args.args[0].cmd ==
Msg.OutAllLinkUpdate.Cmd.EXISTS)
with mock.patch.object(IM.CommandSeq, 'add') as mocked:
test_device.delete_on_device(test_entry_dev1_resp)
assert len(test_device) == 0
assert mocked.call_count == 1
calls = [call(test_device._delete_on_device_post_search,
test_entry_dev1_resp)]
mocked.assert_has_calls(calls)

#-----------------------------------------------------------------------
def test_delete_post_empty(self, test_device, test_entry_dev1_ctrl,
caplog):
# add_on_device(self, entry, on_done=None)
# test delete where no entry is on modem
test_device._delete_on_device_post_search(test_entry_dev1_ctrl)
assert "Entry was not on modem" in caplog.text

#-----------------------------------------------------------------------
def test_delete_post_one(self, test_device, test_entry_dev1_ctrl,
caplog):
# add_on_device(self, entry, on_done=None)
# test delete of a single entry
test_device.add_entry(test_entry_dev1_ctrl)
test_device._delete_on_device_post_search(test_entry_dev1_ctrl)
test_device.delete_on_device(test_entry_dev1_resp)
test_device.delete_on_device(test_entry_dev1_ctrl)
assert (test_device.device.protocol.sent[0].msg.cmd ==
Msg.OutAllLinkUpdate.Cmd.DELETE)
db_flags = Msg.DbFlags.from_bytes(bytes(1))
assert (test_device.device.protocol.sent[0].msg.to_bytes() ==
Msg.OutAllLinkUpdate(Msg.OutAllLinkUpdate.Cmd.DELETE, db_flags,
test_entry_dev1_ctrl.group,
test_entry_dev1_ctrl.addr,
bytes(3)).to_bytes())

#-----------------------------------------------------------------------
def test_delete_post_two_first(self, test_device, test_entry_dev1_ctrl,
test_entry_dev1_resp, caplog):
# add_on_device(self, entry, on_done=None)
# test deleting the first of two entries
test_device.add_entry(test_entry_dev1_ctrl)
test_device.add_entry(test_entry_dev1_resp)
test_device._delete_on_device_post_search(test_entry_dev1_ctrl)
assert (test_device.device.protocol.sent[0].msg.cmd ==
assert (test_device.device.protocol.sent[0].msg.db_flags.to_bytes() ==
Msg.DbFlags(in_use=True, is_controller=False,
is_last_rec=False).to_bytes())
assert (test_device.device.protocol.sent[0].msg.addr ==
test_entry_dev1_resp.addr)
assert (test_device.device.protocol.sent[0].msg.group ==
test_entry_dev1_resp.group)
assert (test_device.device.protocol.sent[1].msg.cmd ==
Msg.OutAllLinkUpdate.Cmd.DELETE)
db_flags = Msg.DbFlags.from_bytes(bytes(1))
sent = test_device.device.protocol.sent[0]
assert (sent.msg.to_bytes() ==
Msg.OutAllLinkUpdate(Msg.OutAllLinkUpdate.Cmd.DELETE, db_flags,
test_entry_dev1_ctrl.group,
test_entry_dev1_ctrl.addr,
bytes(3)).to_bytes())
assert len(sent.handler.next) == 0
assert (test_device.device.protocol.sent[1].msg.db_flags.to_bytes() ==
Msg.DbFlags(in_use=True, is_controller=True,
is_last_rec=False).to_bytes())

#-----------------------------------------------------------------------
def test_delete_post_two_second(self, test_device, test_entry_dev1_ctrl,
test_entry_dev1_resp, caplog):
# add_on_device(self, entry, on_done=None)
# test deleting the second of two entries
test_device.add_entry(test_entry_dev1_ctrl)
test_device.add_entry(test_entry_dev1_resp)
test_device._delete_on_device_post_search(test_entry_dev1_resp)
assert (test_device.device.protocol.sent[0].msg.cmd ==
Msg.OutAllLinkUpdate.Cmd.DELETE)
db_flags = Msg.DbFlags.from_bytes(bytes(1))
sent = test_device.device.protocol.sent[0]
assert (sent.msg.to_bytes() ==
Msg.OutAllLinkUpdate(Msg.OutAllLinkUpdate.Cmd.DELETE, db_flags,
test_entry_dev1_resp.group,
test_entry_dev1_resp.addr,
bytes(3)).to_bytes())
assert len(sent.handler.next) == 2
assert (sent.handler.next[0][0].to_bytes() ==
Msg.OutAllLinkUpdate(Msg.OutAllLinkUpdate.Cmd.DELETE, db_flags,
test_entry_dev1_resp.group,
test_entry_dev1_resp.addr,
bytes(3)).to_bytes())
db_flags = Msg.DbFlags(in_use=True,
is_controller=test_entry_dev1_ctrl.is_controller,
is_last_rec=False)
assert (sent.handler.next[1][0].to_bytes() ==
Msg.OutAllLinkUpdate(Msg.OutAllLinkUpdate.Cmd.ADD_CONTROLLER,
db_flags, test_entry_dev1_ctrl.group,
test_entry_dev1_ctrl.addr,
test_entry_dev1_ctrl.data).to_bytes())

#-----------------------------------------------------------------------
2 changes: 1 addition & 1 deletion tests/handler/test_ModemDbModify.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def test_delete_nak(self, test_db, test_entry_dev1_ctrl, caplog):
data=None, is_ack=False)
handler.msg_received(test_db.device.protocol, msg)
assert len(test_db) == 1
assert "db update failed" in caplog.text
assert "db delete failed" in caplog.text

def test_update(self, test_db, test_entry_dev1_ctrl,
test_entry_dev1_ctrl_mod):
Expand Down

0 comments on commit 139aa11

Please sign in to comment.